Closed Bug 1250897 Opened 9 years ago Closed 9 years ago

Add detailed number data in Telemetry pings

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: gfritzsche, Assigned: Sylvestre, Mentored)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file)

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
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
(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)
Sylvestre wanted to look into this.
Assignee: nobody → sledru
Mentor: gfritzsche
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)
Summary: Missing build numbers in Telemetry → Add build number data in Telemetry pings
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
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?
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)
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 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)
Attachment #8723998 - Flags: review?(gfritzsche)
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/
Attachment #8723998 - Flags: review?(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.
Attachment #8723998 - Flags: review?(gfritzsche)
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 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)
Attachment #8723998 - Flags: review?(gfritzsche)
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 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)
Attachment #8723998 - Flags: review+
Comment on attachment 8723998 [details] MozReview Request: Bug 1250897 - Add build number data in Telemetry pings https://reviewboard.mozilla.org/r/36807/#review33417
NI Benjamin for comment #15.
Flags: needinfo?(benjamin)
Blocks: 1229682
Summary: Add build number data in Telemetry pings → Add detailed number data in Telemetry pings
This is fine. Please make sure that this gets added to the schemas.
Flags: needinfo?(benjamin)
> Please make sure that this gets added to the schemas. Georg, as discussed on irc
Flags: needinfo?(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
Flags: needinfo?(gfritzsche)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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?
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: