Add detailed number data in Telemetry pings

RESOLVED FIXED in Firefox 46

Status

()

P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gfritzsche, Assigned: sylvestre, Mentored)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
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
(Reporter)

Comment 2

3 years ago
(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?
Flags: needinfo?(benjamin)
(Reporter)

Comment 3

3 years ago
Sylvestre wanted to look into this.
Assignee: nobody → sledru
Mentor: gfritzsche

Comment 4

3 years ago
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.
Flags: needinfo?(benjamin)
(Reporter)

Updated

3 years ago
Summary: Missing build numbers in Telemetry → Add build number data in Telemetry pings
(Assignee)

Comment 5

3 years ago
Created attachment 8723998 [details]
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/
(Assignee)

Comment 6

3 years ago
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/
Attachment #8723998 - Attachment description: MozReview Request: Bug 1250897 - Add build number data in Telemetry pings → MozReview Request: Bug 1250897 - Add build number data in Telemetry pings r?=gfritzsche
(Assignee)

Comment 7

3 years ago
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?
(Assignee)

Updated

3 years ago
Attachment #8723998 - Attachment description: MozReview Request: Bug 1250897 - Add build number data in Telemetry pings r?=gfritzsche → MozReview Request: Bug 1250897 - Add build number data in Telemetry pings
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Comment 8

3 years ago
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/
(Reporter)

Comment 9

3 years ago
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("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
> +  loadAddonManager("xpcshell@tests.mozilla.org", "XPCShell", "1b2", "1.9.2");

This version here is different from the "display version", we should not change it.
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Updated

3 years ago
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

3 years ago
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/
(Reporter)

Updated

3 years ago
Attachment #8723998 - Flags: review?(gfritzsche)
(Reporter)

Comment 11

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Comment 12

3 years ago
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/
(Reporter)

Comment 13

3 years ago
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.
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Updated

3 years ago
Attachment #8723998 - Flags: review?(gfritzsche)
(Assignee)

Comment 14

3 years ago
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/
(Reporter)

Comment 15

3 years ago
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
Attachment #8723998 - Flags: review?(gfritzsche)
(Reporter)

Updated

3 years ago
Attachment #8723998 - Flags: review+
(Reporter)

Comment 16

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

https://reviewboard.mozilla.org/r/36807/#review33417
(Assignee)

Comment 17

3 years ago
NI Benjamin for comment #15.
Flags: needinfo?(benjamin)
(Assignee)

Updated

3 years ago
Blocks: 1229682
(Assignee)

Updated

3 years ago
Summary: Add build number data in Telemetry pings → Add detailed number data in Telemetry pings

Comment 18

3 years ago
This is fine. Please make sure that this gets added to the schemas.
Flags: needinfo?(benjamin)
(Assignee)

Comment 19

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

Georg, as discussed on irc
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 21

3 years ago
(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
Flags: needinfo?(gfritzsche)

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06a4669aaf8c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 23

3 years ago
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
Attachment #8723998 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
status-firefox46: --- → affected
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
Attachment #8723998 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 25

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f3401ec1455f
status-firefox46: affected → fixed
You need to log in before you can comment on or make changes to this bug.