1250897 - Add detailed number data in Telemetry pings

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

For pre-release channels like beta, we have a "bN" or "aN" suffix (e.g. "45.0b6" vs. "45.0").

This is called the "display version" and we don't have this data point anywhere in Telemetry.
Bug 1240522 and other use-cases looking at specific betas would need this (to avoid doing buildid-based lookups).

This data seems to come from browser/config/version_display.txt & browser/config/version.txt:
* https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/configure.in#1764
* http://hg.mozilla.org/releases/mozilla-beta/file/tip/browser/config/version.txt
* http://hg.mozilla.org/releases/mozilla-beta/file/tip/browser/config/version_display.txt

Comment 1

[Georg Fritzsche [:gfritzsche]]

We need this data point in more than just the "main" ping (e.g. "activation" and "upgrade").
Consequently, i think we should put this into the ping.application section [0].

I would say this could be called "buildNo", but Sylvestre mentioned that part in the display version is not only used for build numbers.

0: https://dxr.mozilla.org/mozilla-central/rev/5b2baa5e9356644a7ed0b73e422eaff62e159ffb/toolkit/components/telemetry/TelemetryController.jsm#362

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> We need this data point in more than just the "main" ping (e.g. "activation"
> and "upgrade").
> Consequently, i think we should put this into the ping.application section
> [0].
> 
> I would say this could be called "buildNo", but Sylvestre mentioned that
> part in the display version is not only used for build numbers.

Benjamin, are you ok with putting this info into the application section?

Comment 3

[Georg Fritzsche [:gfritzsche]]

Sylvestre wanted to look into this.

Comment 4

[Benjamin Smedberg]

I'm fine with it. My understanding is that we do this mapping server-side for crash-stats, so that might be a reasonable stopgap as well if there's something that this is urgently blocking.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review commit: https://reviewboard.mozilla.org/r/36807/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36807/

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36807/diff/1-2/

Comment 7

[Sylvestre Ledru [:Sylvestre]]

https://reviewboard.mozilla.org/r/36807/#review33361

First version, this is not fully working as the test is failing
AppConstants.MOZ_APP_VERSION_DISPLAY is provided by the local application. Here, it is a1. 
Should I move AppConstants.MOZ_APP_VERSION_DISPLAY into Services.appinfo?

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36807/diff/2-3/

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

https://reviewboard.mozilla.org/r/36807/#review33389

Lets also add documentation for the new field in `common-ping.rst`.

::: toolkit/components/telemetry/TelemetryController.jsm:392
(Diff revision 3)
> +      detailedVersion: detailed,

Per IRC discussion, lets just submit the display version directly here (e.g. "45.0b6") as `displayVersion`.
The solution here will behave a bit inconsistent and doesn't seem well-behaved for analysis.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js:27
(Diff revision 3)
> -const APP_VERSION = "1";
> +const APP_VERSION = "1b2";

This version here is different from the "display version", we should not change it.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js:62
(Diff revision 3)
> +    detailedVersion: APP_DETAILED_VERSION,

With the changes above we can just check against AppConstants here.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js:99
(Diff revision 3)
> -  loadAddonManager("[e-mail]", "XPCShell", "1", "1.9.2");
> +  loadAddonManager("[e-mail]", "XPCShell", "1b2", "1.9.2");

This version here is different from the "display version", we should not change it.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36807/diff/3-4/

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

https://reviewboard.mozilla.org/r/36807/#review33401

Thanks, this looks good except for the test update.

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
(Diff revisions 3 - 4)
> -    detailedVersion: APP_DETAILED_VERSION,

We can compare displayVersion against AppConstants.MOZ_APP_VERSION_DISPLAY here instead of the changes below.
While that is not the most sophisticated testing, it gives us some sanity-checking.

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36807/diff/4-5/

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

https://reviewboard.mozilla.org/r/36807/#review33405

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js:74
(Diff revisions 4 - 5)
> +  Assert.equal(aPing.application.displayVersion, AppConstants.MOZ_APP_VERSION_DISPLAY,

Let's just put this into the `APPLICATION_TEST_DATA` above, no need for a separate assertion here.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36807/diff/5-6/

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

https://reviewboard.mozilla.org/r/36807/#review33415

Thanks, this looks good.
We need Benjamin to have a quick look (per [0]), but per the previous comment we should be fine here.

0: https://wiki.mozilla.org/Firefox/Data_Collection

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

https://reviewboard.mozilla.org/r/36807/#review33417

Comment 17

[Sylvestre Ledru [:Sylvestre]]

NI Benjamin for comment #15.

Comment 18

[Benjamin Smedberg]

This is fine. Please make sure that this gets added to the schemas.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

> Please make sure that this gets added to the schemas.

Georg, as discussed on irc

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06a4669aaf8c

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> > Please make sure that this gets added to the schemas.
> 
> Georg, as discussed on irc

PR is up: https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/1

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06a4669aaf8c

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Approval Request Comment
[Feature/regressing bug #]: Not a new issue, we want data sooner
[User impact if declined]: We will have to wait longer to have better partial for beta builds. See bug 1229682
[Describe test coverage new/current, TreeHerder]: Nightly has the new field
[Risks and why]: New field in an existing structure + has a test covering the change
[String/UUID change made/needed]: None

Comment 24

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8723998 [details]
MozReview Request: Bug 1250897 - Add build number data in Telemetry pings

Better data, so useful, adds in build/version. 
Please uplift to aurora

Comment 25

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3401ec1455f

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3401ec1455f