Closed Bug 1461772 Opened 2 years ago Closed 2 years ago

Fennec core pings do not contain enough information to easily distinguish betas

Categories

(Firefox for Android :: Metrics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 62
Tracking Status
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(2 files)

We submit the application version (e.g. `61.0`) for Fennec (inside the url), but not the display version (e.g. `61.0b4`) which is what we need to be able to distinguish betas. This is needed for telemetry-streaming to be able to set the right set of dimensions on this type of data.

Discussing with :frank on #missioncontrol, the right thing seems to be to add a new field to the core ping with this information.

https://mozilla.logbot.info/missioncontrol/20180515#c14759562
I'll take this one.
Assignee: nobody → wlachance
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

Ok, try run looks good.

:gsvelto, could you review this? I know this isn't exactly your area but you seem to have experience in this general part of the codebase and the feature we're enabling (incorporating the display version into the core ping) is relevant to crashes, since it will allow us to normalize the counts of crash pings for fennec on the beta channel.

:frank: Just making sure this patch was written how you would have imagined it to be.

:chutten: I don't think we need data review for this tiny change, but wanted to double check that with you.
Attachment #8975906 - Flags: review?(gsvelto)
Attachment #8975906 - Flags: feedback?(fbertsch)
Attachment #8975906 - Flags: feedback?(chutten)
Summary: Fennec core pings do not contain enough information to distinguish betas → Fennec core pings do not contain enough information to easily distinguish betas
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

This looks right as I was expecting. It is odd to see both "MOZ_APP_VERSION_DISPLAY" and "DISPLAY_VERSION"; it seems it should be "MOZ_APP_DISPLAY_VERSION" instead - but maybe there's some related work in Desktop-land I'm not aware of.
Attachment #8975906 - Flags: feedback?(fbertsch) → feedback+
(In reply to Frank Bertsch [:frank] from comment #5)
> This looks right as I was expecting. It is odd to see both
> "MOZ_APP_VERSION_DISPLAY" and "DISPLAY_VERSION"; it seems it should be
> "MOZ_APP_DISPLAY_VERSION" instead - but maybe there's some related work in
> Desktop-land I'm not aware of.

It's probably a leftover from ancient times. The corresponding configure variable is called MOZ_APP_VERSION_DISPLAY but the telemetry / crash reporter parameter was always called display version. Most likely all the non-C/C++ in-code constants were made to match the name in the configure script because the corresponding C/C++ define was called MOZ_APP_VERSION_DISPLAY.
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

https://reviewboard.mozilla.org/r/244114/#review250240

LGTM, it's good to see more parity between Fennec and desktop telemetry.
Attachment #8975906 - Flags: review?(gsvelto) → review+
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

https://reviewboard.mozilla.org/r/244114/#review250296

Two things:

1) Yes, it requires Data Collection Review
2) Will this require an upstream schema change?
Attachment #8975906 - Flags: feedback?(chutten)
Attached file data review request
Review request based on previous one submitted with bug 1452745.
Attachment #8976154 - Flags: review?(chutten)
(In reply to Chris H-C :chutten from comment #8)

> 2) Will this require an upstream schema change?

Discussed this with frank. It's probably not a bad idea just to have it in there (with a small sanity check to see that it's reasonably specified if it is present):

https://mozilla.logbot.info/missioncontrol/20180516#c14763965
https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/156
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

https://reviewboard.mozilla.org/r/244114/#review250360

Also: this needs to update the in-tree documentation for the "core" ping https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/data/core-ping.html
Comment on attachment 8976154 [details]
data review request

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes. This will likely be used almost immediately in Mission Control which is well-documented. Also, the "core" ping in-tree documentation will be updated (won't it, wlach? :D )

    Is there a control mechanism that allows the user to turn the data collection on and off?

Standard Telemetry Mechanism

    If the request is for permanent data collection, is there someone who will monitor the data over time?

N/A, this is the addition of a version specifier.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Type 1

    Is the data collection request for default-on or default-off?

Default-on

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

Sorta, but not really. It adds the "display version" identifier which is a sequence of alphanumeric characters set at build time.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data?

No.
---
Result: datareview+
Attachment #8976154 - Flags: review?(chutten) → review+
(In reply to Chris H-C :chutten from comment #11)
> Comment on attachment 8975906 [details]
> Bug 1461772 - Submit display version as a property of the fennec core ping
> 
> https://reviewboard.mozilla.org/r/244114/#review250360
> 
> Also: this needs to update the in-tree documentation for the "core" ping
> https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
> telemetry/data/core-ping.html

Updated, got an r+ on changes from frank on #missioncontrol: https://mozilla.logbot.info/missioncontrol/20180516#c14766393
Pushed by wlachance@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9a05a519ba2a
Submit display version as a property of the fennec core ping r=gsvelto
https://hg.mozilla.org/mozilla-central/rev/9a05a519ba2a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

I would like to uplift this trivial patch to beta so we can start tracking the usage hours on that channel correctly, instead of having to wait for the next nighly merge.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: No user impact
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes, I verified it. I have run a spark job and seen that the field is present on nightly core pings from fennec.
[Needs manual test from QE? If yes, steps to reproduce]: None needed
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Low risk, very minimal change and has been implicitly tested on nightly since it landed
[String changes made/needed]: None
Attachment #8975906 - Flags: approval-mozilla-beta?
Comment on attachment 8975906 [details]
Bug 1461772 - Submit display version as a property of the fennec core ping

Telemetry fix to improve our Fennec data in Mission Control. Approved for Fennec 61.0b9.
Attachment #8975906 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.