Closed Bug 1101487 Opened 10 years ago Closed 9 years ago

20% of Telemetry pings missing clientID field

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: vladan, Assigned: gfritzsche)

References

Details

Attachments

(2 files)

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.
Summary: 20% of Telemetry pings missing clientID → 20% of Telemetry pings missing clientID field
Blocks: 1105864
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)
No, I don't think it's realistic.
Flags: needinfo?(benjamin)
Depends on: 1107686
Depends on: 1107708
vladan, flagging you for sharing the other ideas you had on this issue.
Flags: needinfo?(vdjeric)
(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)
(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.
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
Depends on: 1111707
Preferences.jsm makes this all a bit saner as it allows providing default values.
Attachment #8537237 - Flags: review?(vdjeric)
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+
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+
(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.
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?
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
Attachment #8537237 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1119281
You need to log in before you can comment on or make changes to this bug.