Closed Bug 1119281 Opened 5 years ago Closed 5 years ago

ClientID is missing


(Toolkit :: Telemetry, defect)

Not set



Tracking Status
firefox35 --- unaffected
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- fixed


(Reporter: rvitillo, Assigned: gfritzsche)




(1 file, 1 obsolete file)

It seems that all clientIDs went missing after Bug 1101487 landed, see
There are some clientIDs appearing in "idle-daily" submissions for builds in that time period, though they do appear to be missing in the raw incoming data for "saved-session" pings too.
mreid checked the raw data server-side:
> <mreid> gfritzsche: looks like we're receiving some on "idle-daily" but not on "saved-session"
> <gfritzsche> mreid: strange ok - starting in that timeframe too i assume?
> <mreid> gfritzsche: there are only a small number of submissions from that time range in the raw data I'm looking at, so it's hard to say for sure
> <mreid> but the last "present" one on saved-session so far is from 12/17

I wonder if we do happen to save/serialize pings after we hit TelemetryPing.uninstall() here:
... as we null out the clientID there - which was a change from bug 1101487. The other changes don't seem like they shouldn't affect the clientID submissions negatively.
Blocks: 1101487
Attached patch Fix (obsolete) — Splinter Review
Bug 1101487 made TelemetryPing.uninstall() clear out the clientID.
However, telemetry session save on shutdown happens after uinstall(), so we lost the client id there.

This changes things to not affect this path and adds some test coverage.
AsyncShutdown phase state etc. are hard to reset, so this goes for a specialized testing shutdown path.
Assignee: nobody → gfritzsche
Attachment #8546790 - Flags: review?(vdjeric)
Comment on attachment 8546790 [details] [diff] [review]

Review of attachment 8546790 [details] [diff] [review]:

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +1313,5 @@
>    get clientID() {
>      return this._clientID;
>    },
> +
> +  shutdown: function(testing=false) {

- Write documentation comment above function
- spacing.. testing = false
Attachment #8546790 - Flags: review?(vdjeric) → review+
Blocks: 1069869
Try looks good now.
Keywords: checkin-needed
Attached patch FixSplinter Review
Attachment #8546790 - Attachment is obsolete: true
Attachment #8552642 - Flags: review+
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Mark, can you confirm this fix server-side?
Flags: needinfo?(mreid)
I checked a small sample of incoming pings, and I'm seeing clientIDs in saved-session pings from nightly buildids after 20150123.

Please let me know if further checking is required.
Flags: needinfo?(mreid)
Comment on attachment 8552642 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: Follow-up for bug 1101487.
[User impact if declined]: Missing client id in telemetry saved-session pings
[Describe test coverage new/current, TreeHerder]: improved automated test-coverage, confirmed server-side.
[Risks and why]: Rather low risk of some side-effects on telemetry, but this should have shown up by now.
[String/UUID change made/needed]: None.
Attachment #8552642 - Flags: approval-mozilla-beta?
Attachment #8552642 - Flags: approval-mozilla-aurora?
Attachment #8552642 - Flags: approval-mozilla-beta?
Attachment #8552642 - Flags: approval-mozilla-beta+
Attachment #8552642 - Flags: approval-mozilla-aurora?
Attachment #8552642 - Flags: approval-mozilla-aurora+

Needs rebasing for beta uplift.
Flags: needinfo?(gfritzsche)
Flags: in-testsuite+
I'll wait for confirmation on bug 1128135 before doing a beta uplift.
Flags: needinfo?(gfritzsche)
OS: Mac OS X → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.