Closed
Bug 1101487
Opened 10 years ago
Closed 9 years ago
20% of Telemetry pings missing clientID field
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: vladan, Assigned: gfritzsche)
References
Details
Attachments
(2 files)
5.45 KB,
patch
|
vladan
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
I was doing a Telemetry analysis of DNT_USAGE Telemetry data and I found that a consistent 1 out of 5 pings do not have a clientID field! Specifically, I looked at desktop Firefx nightly/aurora/beta payloads from buildIDs > 20141103, and received during November 3rd to November 18th.
Reporter | ||
Updated•10 years ago
|
Summary: 20% of Telemetry pings missing clientID → 20% of Telemetry pings missing clientID field
Assignee | ||
Comment 1•9 years ago
|
||
From the code-path through TelemetryPing.jsm and setting up the client id, we should be fine. We don't submit a client id though if FHR is disabled. I guess we should add a fhrEnabled property to the ping to verify this. bsmedberg, do we have numbers on how many people have FHR on? Is the 80% implied here realistic?
Flags: needinfo?(benjamin)
Assignee | ||
Comment 3•9 years ago
|
||
vladan, flagging you for sharing the other ideas you had on this issue.
Flags: needinfo?(vdjeric)
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to Georg Fritzsche [:gfritzsche] from comment #3) > vladan, flagging you for sharing the other ideas you had on this issue. My suspicion was that very short sessions (1 or 2 minutes) never fire trigger the timerCallback() function: http://hg.mozilla.org/mozilla-central/annotate/28fdae830289/toolkit/components/telemetry/TelemetryPing.jsm#l1012 It would be very easy to confirm this hypothesis by checking if there is a higher % of pings without clientID in short Telemetry sessions vs longer sessions
Flags: needinfo?(vdjeric)
Comment 5•9 years ago
|
||
(In reply to Vladan Djeric (:vladan) from comment #4) > (In reply to Georg Fritzsche [:gfritzsche] from comment #3) > > vladan, flagging you for sharing the other ideas you had on this issue. > > My suspicion was that very short sessions (1 or 2 minutes) never fire > trigger the timerCallback() function: > http://hg.mozilla.org/mozilla-central/annotate/28fdae830289/toolkit/ > components/telemetry/TelemetryPing.jsm#l1012 > > It would be very easy to confirm this hypothesis by checking if there is a > higher % of pings without clientID in short Telemetry sessions vs longer > sessions That seems to be the case, see http://nbviewer.ipython.org/gist/vitillo/7ad4ac2c4838db09e156 Even though most sessions without clientID have a very short uptime, at least 5% of those have an uptime of over 2 hours though.
Assignee | ||
Comment 6•9 years ago
|
||
Roberto did an analysis on this, which confirms that clientIDs are mostly missing from very short sessions: http://nbviewer.ipython.org/gist/vitillo/7ad4ac2c4838db09e156 The proposed solution is to cache the client id in prefs and use that until we loaded the DRS client id from disk. That may lead to the clientID being stale for one session, but that seems tolerable.
Assignee: nobody → georg.fritzsche
Assignee | ||
Comment 7•9 years ago
|
||
Preferences.jsm makes this all a bit saner as it allows providing default values.
Attachment #8537237 -
Flags: review?(vdjeric)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8537238 -
Flags: review?(vdjeric)
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8537238 [details] [diff] [review] Bonus: Clean up TelemetryPing to use Preferences.jsm Review of attachment 8537238 [details] [diff] [review]: ----------------------------------------------------------------- We don't need to import Preferences.jsm?
Attachment #8537238 -
Flags: review?(vdjeric) → review+
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8537237 [details] [diff] [review] Cache client id in prefs Review of attachment 8537237 [details] [diff] [review]: ----------------------------------------------------------------- Never mind, the Preferences.jsm include is in this patch ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +960,5 @@ > + // For very short session durations, we may never load the client > + // id from disk. > + // We try to cache it in prefs to avoid this, even though this may > + // lead to some stale client ids. > + this._clientID = Preferences.get(PREF_CACHED_CLIENTID, null); So this approach won't completely eliminate missing clientIDs (e.g. if the user only has very short sessions so the pref never gets set) and it will also introduce the possibility of reporting an incorrect (stale) clientID, but I think it's good enough. Eventually, we might want to just synchronously read the clientID from its file during startup. @@ +1009,5 @@ > + // Update cached client id. > + Preferences.set(PREF_CACHED_CLIENTID, this._clientID); > + } else { > + // Nuke potentially cached client id. > + Preferences.reset(PREF_CACHED_CLIENTID); Out of curiosity, when does a profile's FHR clientID change or disappear? ::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js @@ +498,5 @@ > if ("@mozilla.org/datareporting/service;1" in Cc) { > gDataReportingClientID = yield gDatareportingService.getClientID(); > + > + // We should have cached the client id now. Lets confirm that by > + // checking the client id before the async ping setup is finished. Maybe explicitly set TelemtryPing.clientID = null here before proceeding to make sure we're testing the right thing
Attachment #8537237 -
Flags: review?(vdjeric) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Vladan Djeric (:vladan) from comment #10) > So this approach won't completely eliminate missing clientIDs (e.g. if the > user only has very short sessions so the pref never gets set) and it will > also introduce the possibility of reporting an incorrect (stale) clientID, > but I think it's good enough. Right, i think everything else will necessarily have startup perf impacts. > Eventually, we might want to just synchronously read the clientID from its > file during startup. Can we afford that startup per impact? Cant we rather wait for it on shutdown if we need to? > @@ +1009,5 @@ > > + // Update cached client id. > > + Preferences.set(PREF_CACHED_CLIENTID, this._clientID); > > + } else { > > + // Nuke potentially cached client id. > > + Preferences.reset(PREF_CACHED_CLIENTID); > > Out of curiosity, when does a profile's FHR clientID change or disappear? This part is for the (probably exotic corner case) that your profile switches between builds with and without the data reporting service.
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a105575e0da https://hg.mozilla.org/integration/mozilla-inbound/rev/95e2754cb8e7
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8537237 [details] [diff] [review] Cache client id in prefs Approval Request Comment [Feature/regressing bug #]: Telemetry client id, bug 1064333 [User impact if declined]: Telemetry client id missing for short sessions [Describe test coverage new/current, TBPL]: automated test-coverage [Risks and why]: Low-risk, just pref-caching a value. [String/UUID change made/needed]: None.
Attachment #8537237 -
Flags: approval-mozilla-aurora?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a105575e0da https://hg.mozilla.org/mozilla-central/rev/95e2754cb8e7
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Updated•9 years ago
|
status-firefox36:
--- → affected
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8537237 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/71075cc6d27d https://hg.mozilla.org/releases/mozilla-aurora/rev/a0f19d8c4bd6
You need to log in
before you can comment on or make changes to this bug.
Description
•