Closed
Bug 1250897
Opened 9 years ago
Closed 9 years ago
Add detailed number data in Telemetry pings
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: gfritzsche, Assigned: Sylvestre, Mentored)
References
Details
(Whiteboard: [measurement:client])
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
gfritzsche
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details |
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•9 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•9 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•9 years ago
|
||
Sylvestre wanted to look into this.
Assignee: nobody → sledru
Mentor: gfritzsche
Comment 4•9 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•9 years ago
|
Summary: Missing build numbers in Telemetry → Add build number data in Telemetry pings
Assignee | ||
Comment 5•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36807/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36807/
Assignee | ||
Comment 6•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Attachment #8723998 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 10•9 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•9 years ago
|
Attachment #8723998 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 11•9 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•9 years ago
|
Attachment #8723998 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 12•9 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•9 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•9 years ago
|
Attachment #8723998 -
Flags: review?(gfritzsche)
Assignee | ||
Comment 14•9 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•9 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•9 years ago
|
Attachment #8723998 -
Flags: review+
Reporter | ||
Comment 16•9 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 | ||
Updated•9 years ago
|
Summary: Add build number data in Telemetry pings → Add detailed number data in Telemetry pings
Comment 18•9 years ago
|
||
This is fine. Please make sure that this gets added to the schemas.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 19•9 years ago
|
||
> Please make sure that this gets added to the schemas.
Georg, as discussed on irc
Flags: needinfo?(gfritzsche)
Comment 20•9 years ago
|
||
Reporter | ||
Comment 21•9 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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 23•9 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•9 years ago
|
status-firefox46:
--- → affected
Comment 24•9 years ago
|
||
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•9 years ago
|
||
bugherder uplift |
Comment 26•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•