1119281 - ClientID is missing

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Roberto Agostino Vitillo (:rvitillo)]

It seems that all clientIDs went missing after Bug 1101487 landed, see http://nbviewer.ipython.org/gist/vitillo/942eb0931e4b626cca25

Comment 1

[Mark Reid [:mreid]]

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.

Comment 2

[Georg Fritzsche [:gfritzsche]]

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:
https://hg.mozilla.org/mozilla-central/annotate/70de2960aa87/toolkit/components/telemetry/TelemetryPing.jsm#l1166
... 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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix

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.

Comment 4

[Vladan Djeric (:vladan)]

Comment on attachment 8546790 [details] [diff] [review]
Fix

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

Comment 5

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7475b716558d

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This apparently turned xpcshell tests on Windows permafail:
https://treeherder.mozilla.org/logviewer.html#?job_id=5292616&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9739ef7c3d2f

Comment 7

[Georg Fritzsche [:gfritzsche]]

I don't see that failure locally, pushed to try to cross-check:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b4a2bd7ff39
https://tbpl.mozilla.org/?tree=Try&rev=2b4a2bd7ff39

Comment 8

[Georg Fritzsche [:gfritzsche]]

I finally noticed a missing yield in the test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42e0b675961b
https://tbpl.mozilla.org/?tree=Try&rev=42e0b675961b

Comment 9

[Georg Fritzsche [:gfritzsche]]

Try looks good now.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix

Comment 11

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/df2f2df820a8

Comment 12

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/df2f2df820a8

Comment 13

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/df2f2df820a8

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df2f2df820a8

Comment 15

[Georg Fritzsche [:gfritzsche]]

Mark, can you confirm this fix server-side?

Comment 16

[Mark Reid [: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.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8552642 [details] [diff] [review]
Fix

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4445d5ab227f

Needs rebasing for beta uplift.

Comment 19

[Georg Fritzsche [:gfritzsche]]

I'll wait for confirmation on bug 1128135 before doing a beta uplift.

Comment 20

[Georg Fritzsche [:gfritzsche]]

The dashboard recovered after bug 1128135:
http://ec2-50-112-66-71.us-west-2.compute.amazonaws.com:4352/#sandboxes/TelemetryChannelMetrics60DaysAggregator/outputs/TelemetryChannelMetrics60DaysAggregator.nightly.cbuf

I rolled bug 1128135 into the beta patch and landed that:
https://hg.mozilla.org/releases/mozilla-beta/rev/ced08a06e2d1