Per the Telemetry/FHR unification plan [0], we need to add an activation ping: > A ping on first install which includes just the environment data and serves as > a base for future measurement. > There is an open question about how soon after first install this ping should > be sent, and whether that changes our opt-out data-collection strategy at all. [0] https://docs.google.com/a/mozilla.com/document/d/1IGpzsYGi_sq3YFQDAPyKOkU_BKvXAC95fZYA2i4ceVs/

Bug 1120370 - Implement a Telemetry activation ping, r?gfritzsche

Comment on attachment 8680681 [details] MozReview Request: Bug 1120370 - Implement a Telemetry activation ping, r?gfritzsche https://reviewboard.mozilla.org/r/23669/#review21573 I think this is not the ideal place to implement that - getCachedClientID() et al only happen to be triggered from other modules. It would be better to have a defined point at startup where we do this. I suggest doing this early in startup in TelemetrySession.setupChromeProcess() and maybe exposing a getter for "is new client" in ClientID.jsm for checking whether its a new profile. Can we add test coverage for this too?

(In reply to Georg Fritzsche [:gfritzsche] from comment #2) > I suggest doing this early in startup in > TelemetrySession.setupChromeProcess() and maybe exposing a getter for "is > new client" in ClientID.jsm for checking whether its a new profile. I think this fits well in the delayed TelemetrySession initialization: https://dxr.mozilla.org/mozilla-central/rev/f742b9412ed5aace90ad863b276faae0641090a8/toolkit/components/telemetry/TelemetrySession.jsm#1444 We could e.g. trigger this based on a |yield ClientID.isNewClient()| call that triggers the client id loading if needed. The delayed initialization is run even with early shutdown, so that part is fine. One possible concern: if we crash before the delayed initialization runs, we will never trigger that ping.

This isn't ready for implementation yet, AFAIK, since we're still waiting on data definitions from the install attribution project.

This rebases the patch from :bsmedberg and introduces ClientID.isNewClient.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4) > This isn't ready for implementation yet, AFAIK, since we're still waiting on > data definitions from the install attribution project. I haven't heard about that yet. Is there any bug on that? Do you know who drives it?

This is the bug tracking that work. The data design is currently in the hands of Josephine and Chris More. I've asked them for an update in that bug.

This (https://bugzilla.mozilla.org/show_bug.cgi?id=1259614) is the bug tracking that work.

Removing this from the stub attribution tracker as we are putting code in the existing environment ping in bug 1292360.

Hi Georg, this shows up on Fennec program mgmt board. I wonder if this work falls into your team or the Fennec team? If the Fennec team is asked to implement this, can you help the engineer understand what's required here?

The need for this work is specifically for Firefox Desktop. Fennec already has fast visibility of new installs through the "core" ping, so i don't think there is any work here for that team.

> The need for this work is specifically for Firefox Desktop. Fennec already has fast visibility of new installs through the "core" ping, so i don't think there is any work here for that team. We have discovered that we really can't get activations through adjust, so I believe that there is a need for this on mobile. We need advertising ID based activation tied to distribtuion IDs that is not associated with the userid or profile ID for the sole purpose of tracking new browser activations...

Hi Tim and Max, per the comment12 , Could you please kindly share your views if the Telemetry activation ping is feasible on Fennec ? and where to implement the telemetry ? Thank you very much !!

(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #13) > Could you please kindly share your views if the Telemetry activation ping is > feasible on Fennec ? From comment 11, I don't think there are things to do explicitly on Fennec on this bug. Comment 12 didn't disagree that. Please needinfo if otherwise.

I typoed and said Fennec in the first sentence when I meant Firefox. To reiterate: > We have discovered that we really can't get activations through adjust, so I believe that there is a need for this on mobile. > We need advertising ID based activation tied to distribtuion IDs that is not associated with the userid or profile ID for the sole purpose of tracking new browser activations...

Hi Mike, May I know if the advertising ID based activation is what you are looking for Telemetry ? Can you please further confirm ?

Hi mkaply! I'd like to confirm that: You want to track the "advertising ID" when there's only "distribution ID" and no "profile ID" on Android. Is that correct ? What I don't understand is : the user will have a default profile id when they first install the app. So I think what you need is the mapping between "advertising ID" and "distribution ID" and "profile ID" and "Firefox account". And do the filtering on telemetry to get what you expected? Hi mcomella! Please correct me if I'm wrong. I found the code here[1]. Our app will always get a default profile and it's distribution file when app first run. Is that correct? [1]http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#209

(In reply to Nevin Chen [:nechen] from comment #17) > Hi mkaply! > I'd like to confirm that: > You want to track the "advertising ID" when there's only "distribution ID" > and no "profile ID" on Android. > Is that correct ? Yes, that is correct. > What I don't understand is : the user will have a default profile id when > they first install the app. So I think what you need is the mapping between > "advertising ID" and "distribution ID" and "profile ID" and "Firefox > account". And do the filtering on telemetry to get what you expected? The goal is to prevent the mapping between the advertising ID and the profile ID because that would be a privacy issue. The activation ping would be a unique ping only using the advertising ID so as to avoid tying the advertising ID to a unique Firefox profile. We want to send an activation ping with google advertising ID and distribution ID only. > > Hi mcomella! > Please correct me if I'm wrong. I found the code here[1]. Our app will > always get a default profile and it's distribution file when app first run. > Is that correct? > > [1]http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/ > mozilla/gecko/GeckoApplication.java#209

I'd consider adding Barbara Bermes and Frank Bertsch to this thread, since it seems like there are requests that will need to be coordinated with the larger product and data engineering efforts for Mobile beyond the people who are currently involved in this thread. :barbara, :frank

(In reply to Mike Kaply [:mkaply] from comment #18) > (In reply to Nevin Chen [:nechen] from comment #17) > > Hi mkaply! > > I'd like to confirm that: > > You want to track the "advertising ID" when there's only "distribution ID" > > and no "profile ID" on Android. > > Is that correct ? > > Yes, that is correct. > > > What I don't understand is : the user will have a default profile id when > > they first install the app. So I think what you need is the mapping between > > "advertising ID" and "distribution ID" and "profile ID" and "Firefox > > account". And do the filtering on telemetry to get what you expected? > > The goal is to prevent the mapping between the advertising ID and the > profile ID because that would be a privacy issue. > > The activation ping would be a unique ping only using the advertising ID so > as to avoid tying the advertising ID to a unique Firefox profile. > > We want to send an activation ping with google advertising ID and > distribution ID only. > > > > > > Hi mcomella! > > Please correct me if I'm wrong. I found the code here[1]. Our app will > > always get a default profile and it's distribution file when app first run. > > Is that correct? > > > > [1]http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/ > > mozilla/gecko/GeckoApplication.java#209 Hi, just jumping in here. I'll probably be working on the server-side components for a new mobile pings. Can you describe the use case for having these two IDs? Why doesn't core ping suffice?

I will pause the implementation until the requirement is clear. Hi Georg If the activation ping is going to include Android's advertising ID, the timing activation ping will be sent out is important to Android. Since at that time Android's advertising ID may not be ready yet. Maybe we can move the advertising ID information till firstrun.1 event in Android[0].... but since the requirement is not clear, we can discuss it later. [0] https://dxr.mozilla.org/mozilla-central/rev/8ff550409e1d1f8b54f6f7f115545dbef857be0b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#962

Per comment 11, this bug is specifically about a need for Firefox Desktop. For any needs for Fennec, please lets take this to a new bug. I think it will be better to start there with the required data first and not initially assume that this needs to use this activation ping (e.g. you could just add an optional field to the core ping).

Sorry, I was pointed to this as a mobile bug. I'll get something opened.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg Georg, would you mind giving a feedback on the overall design of this? I tried to address the following cases: - Normal flow: starting Firefox and having it open, on the first session, for more than 30 minutes. The ping is sent using the normal Telemetry submission flow. - Early shutdown: starting Firefox and closing it before the 30 minutes mark. The ping is sent using the pingsender. - Crash before 30 minutes: starting Firefox and crashing before we hit 30 minutes. The ping is sent on the next restart.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review137676 ::: toolkit/components/telemetry/TelemetrySession.jsm:1561 (Diff revision 1) > + if (Preferences.get(PREF_NEWPROFILE_PING_ENABLED, false)) { > + // If this is the first session of a new profile, mark the "new-profile" > + // ping for sending. We use a preference so that we can deal with crashes > + // happening on the first run. > + if (TelemetryReportingPolicy.isFirstRun()) { > + Preferences.set(PREF_NEWPROFILE_PING, true); What exactly is the pref for? That is not clear to me from "deal with crashes happening on first run". Note that prefs are not saved to disk if we crash before shutdown.

(In reply to Georg Fritzsche [:gfritzsche] from comment #26) > Comment on attachment 8862415 [details] > Bug 1120370 - Implement the new-profile ping. > > https://reviewboard.mozilla.org/r/134346/#review137676 > > ::: toolkit/components/telemetry/TelemetrySession.jsm:1561 > (Diff revision 1) > > + if (Preferences.get(PREF_NEWPROFILE_PING_ENABLED, false)) { > > + // If this is the first session of a new profile, mark the "new-profile" > > + // ping for sending. We use a preference so that we can deal with crashes > > + // happening on the first run. > > + if (TelemetryReportingPolicy.isFirstRun()) { > > + Preferences.set(PREF_NEWPROFILE_PING, true); > > What exactly is the pref for? > That is not clear to me from "deal with crashes happening on first run". > > Note that prefs are not saved to disk if we crash before shutdown. Ha! That collapses my castle :) I thought prefs were saved as soon as they were set. I guess that we don't need any special care then, since the "first run" pref won't be saved to disk if we crash as well. If it doesn't get saved, we'd end up with hitting the logic again on the next restart.

After discussing this with Georg, we came up with two different strategies for sending the "new-profile" ping. 1) Mark the "new-profile" ping as sent With this strategy we always send a "new-profile" ping, unless we marked it as already sent. If we fail to mark the new-profile ping as sent, the ping is sent again. This solution puts more efforts in making sure the data is sent and can lead to duplicates pings being received. The major drawback of this solution is having to deal with existing users, as these would be sending the "new-profile" pings too. Analysts would need to filter out these pings, as they are not probably useful for our use case. 2) Rely on a "signal" for "this is a new profile" This solution relies on detecting the "first run" on a new profile, by using some sort of "signal". Relying on such signal could lead to under reoprting (e.g. the signal is not present or skipped for some reason) and some duplicates (e.g. broken signal reported twice). One possible signal is the "firstRun" preference being used for powering the data choices infobar too. However, we cannot rely on preferences when crashes happen. If we didn't need to handle the "existing users" case, solution (1) would be the ideal one. However we need to, so we'll go for (2). Open question: what's the best way to ship a "this is a new profile" signal to new profiles (not updates)? @Gijs, since you worked on the profile migration, you might have an opinion on this. How about adding a marker (e.g. a dummy file) to every new profile from the "profile creation code" [1]? Do you have a better idea for a "this is a new profile" signal or where to add it? [1] - http://searchfox.org/mozilla-central/source/toolkit/profile/nsToolkitProfileService.cpp#778

(In reply to Alessio Placitelli [:Dexter] from comment #29) > @Gijs, since you worked on the profile migration, you might have an opinion > on this. How about adding a marker (e.g. a dummy file) to every new profile > from the "profile creation code" [1]? Do you have a better idea for a "this > is a new profile" signal or where to add it? I don't really know, besides some of the more obvious pref-related options. Why can't we use the client id which we use for telemetry? If that is unset, this is a new profile, right? IIRC we copy that data across for Firefox refresh, so that shouldn't confuse it, nor would existing profiles... Adding more IO on startup is a definite no-no given current quantum and photon-perf efforts. Maybe Matt has an idea, as I know he worked on ProfileAge.jsm which seems related. If there is a good reason not to use the client id, he would be the best person to ask.

(In reply to :Gijs from comment #30) > (In reply to Alessio Placitelli [:Dexter] from comment #29) > > @Gijs, since you worked on the profile migration, you might have an opinion > > on this. How about adding a marker (e.g. a dummy file) to every new profile > > from the "profile creation code" [1]? Do you have a better idea for a "this > > is a new profile" signal or where to add it? > > I don't really know, besides some of the more obvious pref-related options. > > Why can't we use the client id which we use for telemetry? If that is unset, > this is a new profile, right? IIRC we copy that data across for Firefox > refresh, so that shouldn't confuse it, nor would existing profiles... We can still crash after generating a new client-id. At the next run, the client-id would be set and the profile wouldn't be seen as "new" anymore. > Adding more IO on startup is a definite no-no given current quantum and > photon-perf efforts. Maybe Matt has an idea, as I know he worked on > ProfileAge.jsm which seems related. If there is a good reason not to use the > client id, he would be the best person to ask. The IO would just be when creating the "profile directory", where we already generate a few files, not on every startup. Do you think this would still be an issue? And is Matt.. MattN?

(In reply to Alessio Placitelli [:Dexter] from comment #31) > (In reply to :Gijs from comment #30) > > (In reply to Alessio Placitelli [:Dexter] from comment #29) > > > @Gijs, since you worked on the profile migration, you might have an opinion > > > on this. How about adding a marker (e.g. a dummy file) to every new profile > > > from the "profile creation code" [1]? Do you have a better idea for a "this > > > is a new profile" signal or where to add it? > > > > I don't really know, besides some of the more obvious pref-related options. > > > > Why can't we use the client id which we use for telemetry? If that is unset, > > this is a new profile, right? IIRC we copy that data across for Firefox > > refresh, so that shouldn't confuse it, nor would existing profiles... > > We can still crash after generating a new client-id. At the next run, the > client-id would be set and the profile wouldn't be seen as "new" anymore. Send the ping at the time where you generate the client id? Logically speaking, if there are these events here: 1) create profile 2) send ping 3) create marker to avoid re-sending ping OR: 1) create profile 2) create some marker to avoid re-sending ping 3) send ping Then in either 'algorithm', if a crash happens between the different events we could end up sending duplicate pings or sending no ping at all. That is, if "crash at arbitrary point in time" is a failure mode we're trying to work around, there is no way to avoid that failure mode altogether - the only choice we get is how we deal with that failure. Either we duplicate the ping or we don't send it at all. Both of those are options in both 'algorithm's - it depends on whether the steps that avoid the re-sending the ping are blocked on the sending of the ping being complete (in which case we guarantee >=1 ping is sent) or not (in which case we guarantee <=1 ping is sent). It doesn't matter what the marker/file is. > > Adding more IO on startup is a definite no-no given current quantum and > > photon-perf efforts. Maybe Matt has an idea, as I know he worked on > > ProfileAge.jsm which seems related. If there is a good reason not to use the > > client id, he would be the best person to ask. > > The IO would just be when creating the "profile directory", where we already > generate a few files, not on every startup. > Do you think this would still be an issue? And is Matt.. MattN? Yes, MattN. I don't understand this though - if we send a ping for a 'new' profile, we need to also stop sending it. Wouldn't that require removing/adding/changing files *after* the ping is sent?

(In reply to :Gijs from comment #32) > (In reply to Alessio Placitelli [:Dexter] from comment #31) > > (In reply to :Gijs from comment #30) > > > (In reply to Alessio Placitelli [:Dexter] from comment #29) > > > > @Gijs, since you worked on the profile migration, you might have an opinion > > > > on this. How about adding a marker (e.g. a dummy file) to every new profile > > > > from the "profile creation code" [1]? Do you have a better idea for a "this > > > > is a new profile" signal or where to add it? > > > > > > I don't really know, besides some of the more obvious pref-related options. > > > > > > Why can't we use the client id which we use for telemetry? If that is unset, > > > this is a new profile, right? IIRC we copy that data across for Firefox > > > refresh, so that shouldn't confuse it, nor would existing profiles... > > > > We can still crash after generating a new client-id. At the next run, the > > client-id would be set and the profile wouldn't be seen as "new" anymore. > > Send the ping at the time where you generate the client id? No, we can't send that at that time. We are constrained to send it either 30 minutes after the startup or at shutdown, whatever happens first. >[...] Both of those are options in both 'algorithm's - > it depends on whether the steps that avoid the re-sending the ping are > blocked on the sending of the ping being complete (in which case we > guarantee >=1 ping is sent) or not (in which case we guarantee <=1 ping is > sent). It doesn't matter what the marker/file is. Yes, I'm aware of that, and trying to make sure that at least a ping gets sent. > > > Adding more IO on startup is a definite no-no given current quantum and > > > photon-perf efforts. Maybe Matt has an idea, as I know he worked on > > > ProfileAge.jsm which seems related. If there is a good reason not to use the > > > client id, he would be the best person to ask. > > > > The IO would just be when creating the "profile directory", where we already > > generate a few files, not on every startup. > > Do you think this would still be an issue? And is Matt.. MattN? > > Yes, MattN. I don't understand this though - if we send a ping for a 'new' > profile, we need to also stop sending it. Wouldn't that require > removing/adding/changing files *after* the ping is sent? Sending happens 30 minutes after Firefox starts, which I don't think can qualify as "startup" phase anymore.

(In reply to Alessio Placitelli [:Dexter] from comment #33) > (In reply to :Gijs from comment #32) > > (In reply to Alessio Placitelli [:Dexter] from comment #31) > > > (In reply to :Gijs from comment #30) > > > > (In reply to Alessio Placitelli [:Dexter] from comment #29) > > > > > @Gijs, since you worked on the profile migration, you might have an opinion > > > > > on this. How about adding a marker (e.g. a dummy file) to every new profile > > > > > from the "profile creation code" [1]? Do you have a better idea for a "this > > > > > is a new profile" signal or where to add it? > > > > > > > > I don't really know, besides some of the more obvious pref-related options. > > > > > > > > Why can't we use the client id which we use for telemetry? If that is unset, > > > > this is a new profile, right? IIRC we copy that data across for Firefox > > > > refresh, so that shouldn't confuse it, nor would existing profiles... > > > > > > We can still crash after generating a new client-id. At the next run, the > > > client-id would be set and the profile wouldn't be seen as "new" anymore. > > > > Send the ping at the time where you generate the client id? > > No, we can't send that at that time. > We are constrained to send it either 30 minutes after the startup or at > shutdown, whatever happens first. OK, this was not obvious from the initial phrasing of your question... However, why do we generate the client ID earlier, then? In particular, I'm assuming that we want this "new profile" ping to be the first telemetry ping we send. Why bother generating the client ID sooner? Just generate it when we're sending this ping, and do not save it unless we've sent the ping. > > > > Adding more IO on startup is a definite no-no given current quantum and > > > > photon-perf efforts. Maybe Matt has an idea, as I know he worked on > > > > ProfileAge.jsm which seems related. If there is a good reason not to use the > > > > client id, he would be the best person to ask. > > > > > > The IO would just be when creating the "profile directory", where we already > > > generate a few files, not on every startup. > > > Do you think this would still be an issue? And is Matt.. MattN? > > > > Yes, MattN. I don't understand this though - if we send a ping for a 'new' > > profile, we need to also stop sending it. Wouldn't that require > > removing/adding/changing files *after* the ping is sent? > > Sending happens 30 minutes after Firefox starts, which I don't think can > qualify as "startup" phase anymore. Sure, but you will need to evaluate whether you will need to do this at startup (as shutdown could happen at any time after that, before the 30 minutes are up). If that evaluation requires extra IO, that means you're doing startup IO. Delaying the extra IO until shutdown actually happens ends up slowing down shutdown instead, which we're also trying to optimize... What this boils down to, IMO, is that we should be piggy-backing on a relevant, extant pref / file / state. ClientID seems to fit those criteria.

(In reply to :Gijs from comment #34) > (In reply to Alessio Placitelli [:Dexter] from comment #33) > > (In reply to :Gijs from comment #32) > > > (In reply to Alessio Placitelli [:Dexter] from comment #31) > > > > (In reply to :Gijs from comment #30) > > > > > (In reply to Alessio Placitelli [:Dexter] from comment #29) > > > > > > @Gijs, since you worked on the profile migration, you might have an opinion > > > > > > on this. How about adding a marker (e.g. a dummy file) to every new profile > > > > > > from the "profile creation code" [1]? Do you have a better idea for a "this > > > > > > is a new profile" signal or where to add it? > > > > > > > > > > I don't really know, besides some of the more obvious pref-related options. > > > > > > > > > > Why can't we use the client id which we use for telemetry? If that is unset, > > > > > this is a new profile, right? IIRC we copy that data across for Firefox > > > > > refresh, so that shouldn't confuse it, nor would existing profiles... > > > > > > > > We can still crash after generating a new client-id. At the next run, the > > > > client-id would be set and the profile wouldn't be seen as "new" anymore. > > > > > > Send the ping at the time where you generate the client id? > > > > No, we can't send that at that time. > > We are constrained to send it either 30 minutes after the startup or at > > shutdown, whatever happens first. > > OK, this was not obvious from the initial phrasing of your question... > > However, why do we generate the client ID earlier, then? In particular, I'm > assuming that we want this "new profile" ping to be the first telemetry ping > we send. Why bother generating the client ID sooner? Just generate it when > we're sending this ping, and do not save it unless we've sent the ping. "new-profile" doesn't necessarily have to be the first ping we send. It potentially is, but nothing prevents other pings from going out before. Moreover, we can't delay the creation of the client-id as it is used to power things other than the "new-profile" ping, e.g. "crash" pings and some main pings that can potentially happen before the "new-profile" ping. > > > > > Adding more IO on startup is a definite no-no given current quantum and > > > > > photon-perf efforts. Maybe Matt has an idea, as I know he worked on > > > > > ProfileAge.jsm which seems related. If there is a good reason not to use the > > > > > client id, he would be the best person to ask. > > > > > > > > The IO would just be when creating the "profile directory", where we already > > > > generate a few files, not on every startup. > > > > Do you think this would still be an issue? And is Matt.. MattN? > > > > > > Yes, MattN. I don't understand this though - if we send a ping for a 'new' > > > profile, we need to also stop sending it. Wouldn't that require > > > removing/adding/changing files *after* the ping is sent? > > > > Sending happens 30 minutes after Firefox starts, which I don't think can > > qualify as "startup" phase anymore. > > Sure, but you will need to evaluate whether you will need to do this at > startup (as shutdown could happen at any time after that, before the 30 > minutes are up). If that evaluation requires extra IO, that means you're > doing startup IO. Delaying the extra IO until shutdown actually happens ends > up slowing down shutdown instead, which we're also trying to optimize... > > What this boils down to, IMO, is that we should be piggy-backing on a > relevant, extant pref / file / state. ClientID seems to fit those criteria. That's the reason why I asked the question in the first place. Since using the client-id generation path doesn't really help in this case, I think this boils down to: - Pretend crashes don't happen before we send the "new-profile" pings and just rely on prefs. - Use some "dummy" file in the profile directory and remove it once the ping is sent (guarantees that we send at least a ping, even if we crash; downside: potential IO hit on shutdown, if that happens early) - @Gijs, any suggestion about other options?

After discussing this with Gijs over IRC, to guarantee that we send at least a ping in case of crashes, it was suggested to use preferences and force saving the prefs using |Services.prefs.save(null);| on idle [1] instead of adding a new file in the profile. Saving the pref happens on the main thread, but it's light-touch compared to doing IO on a different file. In light of this, the proposed implementation slightly changes. We can introduce a new pref "toolkit.telemetry.newprofile.sent", if it's false/not set then we send the new-profile ping and then save the prefs on idle as soon as possible. Otherwise, we do nothing. Georg, what's your take on this? [1] - https://developer.mozilla.org/en-US/Firefox/Performance_best_practices_for_Firefox_fe_engineers#requestIdleCallback

Great, that sounds solid and easier to me.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg Georg, the design changed according to the previous comment. Flagging you for feedback on the new design!

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review138856 I left some implementation notes below. One other thing to call out is that we expect "some" duplicates, mostly due to crashes. Bug 789945 would impact this a bit. That should be ok and can be handled by the data jobs or analysis, but we should ask e.g. mreid for feedback on that specifically. ::: commit-message-e17cb:2 (Diff revision 3) > +Bug 1120370 - Implement the new-profile ping. r? > + Can you summarize the (very high-level) mechanics here? That would be help anyone reviewing or later investigations. ::: toolkit/components/telemetry/TelemetrySession.jsm:70 (Diff revision 3) > const PREF_PREVIOUS_BUILDID = PREF_BRANCH + "previousBuildID"; > const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled"; > const PREF_ASYNC_PLUGIN_INIT = "dom.ipc.plugins.asyncInit.enabled"; > const PREF_UNIFIED = PREF_BRANCH + "unified"; > const PREF_SHUTDOWN_PINGSENDER = PREF_BRANCH + "shutdownPingSender.enabled"; > +const PREF_NEWPROFILE_PING_ENABLED = PREF_BRANCH + "newProfilePing.enabled"; Is there a reason why this implementation is in TelemetrySession.jsm? This seems independent of the main ping. Maybe TelemetryController can trigger this for now? ::: toolkit/components/telemetry/TelemetrySession.jsm:728 (Diff revision 3) > _nextTotalMemoryId: 1, > _USSFromChildProcesses: null, > _lastEnvironmentChangeDate: 0, > + // The task performing the delayed sending of the "new-profile" ping. > + _delayedNewPingTask: null, > + _shuttingDown: false, Do we need to add a flag here? I imagine we could: - on delayed startup, if new ping is needed, launch a `DeferredTask` for it - on shutdown, if that task is present, `.finalize()` & wait on it ::: toolkit/components/telemetry/TelemetrySession.jsm:2231 (Diff revision 3) > + // Generate and send the "new-profile" ping. This uses the > + // pingsender if we're shutting down. > + let options = { > + addClientId: true, > + addEnvironment: true, > + usePingSender: this._shuttingDown, Shouldn't TelemetrySend know when to use the ping sender? ::: toolkit/components/telemetry/TelemetrySession.jsm:2238 (Diff revision 3) > + // Flush the prefs if we're not shutting down. Prefs are saved to disk > + // at shutdown anyway. Does that still hold with shutdown timeouts etc.? ::: toolkit/components/telemetry/TelemetrySession.jsm:2243 (Diff revision 3) > + // Flush the prefs if we're not shutting down. Prefs are saved to disk > + // at shutdown anyway. > + if (!this._shuttingDown) { > + // TODO: we need to be smarter about when to save the prefs to file. > + // |requestIdleCallback| is currently only accessible through DOM. We > + // need to change this when bug 1353206 or bug 1358476 land. Please file a follow-up bug that depends on those.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review138856 > Do we need to add a flag here? > I imagine we could: > - on delayed startup, if new ping is needed, launch a `DeferredTask` for it > - on shutdown, if that task is present, `.finalize()` & wait on it Yeah, that is merely needed for the "reason" field of the "new-profile" ping, to detect if this ping was triggered due to a shutdown or after 30 minutes. > Shouldn't TelemetrySend know when to use the ping sender? It doesn't, TelemetrySend just reads the options field and decides how to send the stuff. However, if we were to change TelemetrySend to automatically use the pingsender when shutting down, this would also make other pings (e.g. saved-session) use it. > Does that still hold with shutdown timeouts etc.? Good point. Actually, after looking more carefully into that, Telemetry shutdown happens after [the final pref write](http://searchfox.org/mozilla-central/rev/b0e1da2a90ada7e00f265838a3fafd00af33e547/modules/libpref/Preferences.cpp#636), which happens "profile-before-change". So yeah, we should do it again here if required. > Please file a follow-up bug that depends on those. Filed bug 1361996.

(In reply to Georg Fritzsche [:gfritzsche] from comment #40) > Comment on attachment 8862415 [details] > Bug 1120370 - Implement the new-profile ping. > > https://reviewboard.mozilla.org/r/134346/#review138856 > > I left some implementation notes below. > > One other thing to call out is that we expect "some" duplicates, mostly due > to crashes. Bug 789945 would impact this a bit. > That should be ok and can be handled by the data jobs or analysis, but we > should ask e.g. mreid for feedback on that specifically. Mark, as Georg mentioned, we expect "some" duplicates coming in for the "new-profile" ping. By some, I really claim that's a very low amount of duplicates: they might be triggered by crashes happening right after a ping is sent and before the pref flushing operation completes (and this operation is scheduled to happen right after sending the ping). What's your take on this? Could this simply be taken care of on the analysis side?

(In reply to Alessio Placitelli [:Dexter] from comment #41) > Comment on attachment 8862415 [details] > > Do we need to add a flag here? > > I imagine we could: > > - on delayed startup, if new ping is needed, launch a `DeferredTask` for it > > - on shutdown, if that task is present, `.finalize()` & wait on it > > Yeah, that is merely needed for the "reason" field of the "new-profile" > ping, to detect if this ping was triggered due to a shutdown or after 30 > minutes. Wouldn't you know that from the user agent field already? If the pingsender sent it, it is from shutdown. > > Shouldn't TelemetrySend know when to use the ping sender? > > It doesn't, TelemetrySend just reads the options field and decides how to > send the stuff. However, if we were to change TelemetrySend to automatically > use the pingsender when shutting down, this would also make other pings > (e.g. saved-session) use it. We need at least a follow-up on this. You don't need to enable it for all pings. You can keep a flag that means "send immediately if shutting down" or keep an internal list of these in TelemetrySend. On TelemetrySend shutdown you check outstanding pings and use the pingsender where required.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review139210 Approach is good, a few issues: ::: toolkit/components/telemetry/TelemetryController.jsm:1012 (Diff revision 4) > + const sendDelay = > + Preferences.get(PREF_NEWPROFILE_PING_DELAY, NEWPROFILE_PING_DEFAULT_DELAY); > + > + this._delayedNewPingTask = new DeferredTask(function* () { > + try { > + yield this.sendNewProfilePing(); TIL arrow functions cannot be used as generators ::: toolkit/components/telemetry/TelemetryController.jsm:1043 (Diff revision 4) > + }; > + await TelemetryController.submitExternalPing("new-profile", payload, options) > + .catch(e => this._log.error("sendNewProfilePing - failed to submit new-profile ping", e)); > + > + // Set the "sent" pref so we don't end up sending this ping again. > + Preferences.set(PREF_NEWPROFILE_PING_SENT, true); Should we only set the preference if we didn't catch an error on submitExternalPing? ::: toolkit/components/telemetry/TelemetryController.jsm:1045 (Diff revision 4) > + .catch(e => this._log.error("sendNewProfilePing - failed to submit new-profile ping", e)); > + > + // Set the "sent" pref so we don't end up sending this ping again. > + Preferences.set(PREF_NEWPROFILE_PING_SENT, true); > + // Flush the prefs if we're not shutting down. > + // TODO: we need to be smarter about when to save the prefs to file. Reference bug 1361996 directly in this comment? ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:6 (Diff revision 4) > + > +"new-profile" ping > +================== > + > +This opt-out ping is sent from Firefox Desktop 30 minutes after the browser is started, on the first session > +of a newly created profile. If the ping wasn't sent already, it gets sent using the pingsender at I think clearer wording might be: "If the first session of a newly-created profile was shorter than 30 minutes, it gets sent using the pingsender at shutdown" Also, should we link to the pingsender doc here? ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:11 (Diff revision 4) > +of a newly created profile. If the ping wasn't sent already, it gets sent using the pingsender at > +shutdown. > + > +.. note:: > + > + We don't sent the ping immediately after Telemetry complete initialization to give the user enough Should be either "Telemetry's complete" or "Telemetry completes" depending on your desired meaning. ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:12 (Diff revision 4) > +shutdown. > + > +.. note:: > + > + We don't sent the ping immediately after Telemetry complete initialization to give the user enough > + time to tweak its data collection preferences. user is "they/them/their" not "it/its" ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:24 (Diff revision 4) > + type: "new-profile", > + ... common ping data > + clientId: <UUID>, > + environment: { ... }, > + payload: { > + reason: "startup" or shutdown. ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:35 (Diff revision 4) > +If this field contains ``startup``, then the ping was generated at the scheduled time after > +startup. If it contains ``shutdown``, then the browser was closed before the time the > +ping was scheduled. In the latter case, the ping is generated during shutdown and sent > +using the :doc:`../internals/pingsender`. > + > +Duplicates pings should be "Duplicate pings" ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:39 (Diff revision 4) > + > +Duplicates pings > +---------------- > +We expect a low fraction of duplicates of this ping, mostly due to crashes happening > +right after sending the ping and before the preferences get flushed to the disk. This should > +be fairly low in practices and manageable during the analysis phase. s/practices/practice/

(In reply to Georg Fritzsche [:gfritzsche] from comment #44) > (In reply to Alessio Placitelli [:Dexter] from comment #41) > > Comment on attachment 8862415 [details] > > > Do we need to add a flag here? > > > I imagine we could: > > > - on delayed startup, if new ping is needed, launch a `DeferredTask` for it > > > - on shutdown, if that task is present, `.finalize()` & wait on it > > > > Yeah, that is merely needed for the "reason" field of the "new-profile" > > ping, to detect if this ping was triggered due to a shutdown or after 30 > > minutes. > > Wouldn't you know that from the user agent field already? > If the pingsender sent it, it is from shutdown. As discussed over IRC, this could be problematic in case we generate the "new-profile" ping at shutdown and then we fail to send it using the pingsender (e.g. OS shuts down and fails to spawn the pingsender). In that case the "new-profile" ping would indeed be generated at shutdown, but would not have the pingsender user agent (because it would be sent next time FF restarts). > > > Shouldn't TelemetrySend know when to use the ping sender? > > > > It doesn't, TelemetrySend just reads the options field and decides how to > > send the stuff. However, if we were to change TelemetrySend to automatically > > use the pingsender when shutting down, this would also make other pings > > (e.g. saved-session) use it. > > We need at least a follow-up on this. > You don't need to enable it for all pings. > You can keep a flag that means "send immediately if shutting down" or keep > an internal list of these in TelemetrySend. > On TelemetrySend shutdown you check outstanding pings and use the pingsender > where required. Filed bug 1120370.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review139210 > TIL arrow functions cannot be used as generators Ha! And TIL what TIL means :-D

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg data-review? Benjamin, this lands the "new-profile" opt-out ping that gets sent on fresh profiles, after 30 minutes or at shutdown (whatever comes first). The documentation for this ping lives in "new-profile-ping.rst".

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review139252

(In reply to Alessio Placitelli [:Dexter] from comment #43) > (In reply to Georg Fritzsche [:gfritzsche] from comment #40) > > One other thing to call out is that we expect "some" duplicates, mostly due > > to crashes. Bug 789945 would impact this a bit. > > That should be ok and can be handled by the data jobs or analysis, but we > > should ask e.g. mreid for feedback on that specifically. > > Mark, as Georg mentioned, we expect "some" duplicates coming in for the > "new-profile" ping. By some, I really claim that's a very low amount of > duplicates: they might be triggered by crashes happening right after a ping > is sent and before the pref flushing operation completes (and this operation > is scheduled to happen right after sending the ping). > > What's your take on this? Could this simply be taken care of on the analysis > side? Generally we cannot efficiently detect and remove all duplicates at ingestion time, so yes, robust analyses should be expected to drop duplicates over whatever time period is of interest. That said, we have monitoring in place for the observed rate of duplicates by document type, so at least we can observe and alert if the rate increases past some threshold. We are also starting to make some attempt to drop duplicates at ingestion time, balancing the effectiveness with the risk of rejecting non-duplicates. More context in bug 1342111. Overall, a small number of duplicate submissions should be acceptable.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review140164 data-r=me ::: commit-message-e17cb:9 (Diff revision 5) > +from the Firefox startup. The ping is eventually sent at shutdown > +if the scheduled time wasn't reached. > +To reliably prevent sending the ping more than once, a pref is > +used to keep track of the "sent" state. > + > +The "new-profile" ping is protected behind a pref. This commit message should indicate whether at the time of landing this pref was enabled or disabled by default. ::: toolkit/components/telemetry/docs/internals/preferences.rst:61 (Diff revision 5) > > +``toolkit.telemetry.newProfilePing.enabled`` > + > + Enable the :doc:`../data/new-profile` ping on new profiles. > + > +``toolkit.telemetry.newProfilePing.sent`` Is this going to end up sending a new-profile ping for all existing clients when they upgrade to the version where this is enabled? That seems undesirable.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #52) > ::: toolkit/components/telemetry/docs/internals/preferences.rst:61 > (Diff revision 5) > > > > +``toolkit.telemetry.newProfilePing.enabled`` > > + > > + Enable the :doc:`../data/new-profile` ping on new profiles. > > + > > +``toolkit.telemetry.newProfilePing.sent`` > > Is this going to end up sending a new-profile ping for all existing clients > when they upgrade to the version where this is enabled? That seems > undesirable. Thanks for the review Benjamin. Good point. I've fixed this now (we're piggy backing the "sent" state in the telemetry state file).

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg Georg, would you take another feedback pass on this? We're no longer using prefs but rather piggy backing the "new-profile" ping sent state in the telemetry state file (which doesn't get reset on profile resets and migrations). This allows us to: - prevent to suddenly send the new-profile pings from all the clients (old & new) when they update to a version enabling this; - prevent to send the ping again if the profile is reset.

Comment on attachment 8862415 [details] Bug 1120370 - Implement the new-profile ping. , data-r=bsmedberg https://reviewboard.mozilla.org/r/134346/#review141514 This looks solid from a high-level look, thanks. Some comments below. You should also update `concepts/pings.rst`, we still talk about the `activation` ping there. ::: commit-message-e17cb:6 (Diff revisions 5 - 7) > -To reliably prevent sending the ping more than once, a pref is > -used to keep track of the "sent" state. > +To reliably prevent sending the ping more than once, the telemetry state > +file is used to keep track of the "sent" state. "session-state.json file" ::: toolkit/components/telemetry/TelemetryController.jsm:1039 (Diff revisions 5 - 7) > let options = { > addClientId: true, > addEnvironment: true, > usePingSender: this._shuttingDown, > }; > + // TODO: we need to be smarter about when to save the state to file. We probably should be smarter about when to trigger the ping. The saving of the state file should probably still happen right after. ::: toolkit/components/telemetry/docs/data/new-profile-ping.rst:38 (Diff revisions 5 - 7) > using the :doc:`../internals/pingsender`. > > Duplicate pings > --------------- > We expect a low fraction of duplicates of this ping, mostly due to crashes happening > -right after sending the ping and before the preferences get flushed to the disk. This should > +right after sending the ping and before the telemetry state get flushed to the disk. This should "gets" ::: toolkit/components/telemetry/TelemetrySession.jsm:736 (Diff revision 7) > + // We piggy-back the "new-profile" ping sent flag into the state file, to > + // survive profile refresh and migrations. "We save whether the "new-profile" ping was sent yet ..."? ::: toolkit/components/telemetry/TelemetrySession.jsm:2117 (Diff revision 7) > // new subsession while loading still takes place. This will always be exactly > // 1 - the current subsessions. > this._profileSubsessionCounter = data.profileSubsessionCounter + > this._subsessionCounter; > + // If we don't have this flag in the state file, it means that this is an old profile. > + // We didn't have this flag before, so just set it to true. "set it to true" i can see in the code. What does that actually mean, what do we want to achieve?

Pushed by alessio.placitelli@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1f03ad361009 Implement the new-profile ping. r=bsmedberg,chutten, data-r=bsmedberg

Madalin, this was merged and should be in today's Nightly (that will be pushed to clients during the evening). Could you take care of the QA here?

Yes, I'm on it :)

The schema can be found in the PR here: https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/48

I have created a document to track the testing progress. All the results so far can be found here: https://docs.google.com/spreadsheets/d/1sNfrZaJEvTuNAfAeK3_IjBq6lNtXt73YVH7Vz5dCrlc/edit#gid=0

Marking the bug as verified fixed. The test plan can be accessed in here: https://docs.google.com/document/d/1THuy92VmgzNx7ioYgAP3e8jkUQn8CWShRm1HqcNE4Ho/edit All the testing results are available in here: https://docs.google.com/spreadsheets/d/1sNfrZaJEvTuNAfAeK3_IjBq6lNtXt73YVH7Vz5dCrlc/edit#gid=0