Convert a11y_consumers and a11y_instantiated_flag to opt-out

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Blocks: 2 bugs)

47 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: aes+)

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

a year ago
Created attachment 8888518 [details] [diff] [review]
patch
Attachment #8888518 - Flags: review?(dbolter)
(Assignee)

Comment 2

a year ago
Comment on attachment 8888518 [details] [diff] [review]
patch

For data review. We'd like to collect accurate client counts out on release for these probes. The instantiated flag is a bool, no privacy issue there afaict. The consumers probe is an enum that detects injected dlls to identify specific clients, which is no different from the recent opt-out consumer scalar we landed.
Attachment #8888518 - Flags: review?(benjamin)
Attachment #8888518 - Flags: review?(dbolter) → review+
Priority: -- → P3

Comment 3

a year ago
Comment on attachment 8888518 [details] [diff] [review]
patch

There are a few blocking issues and a bunch of nonblocking issues that should be followups:

Blocking issues:
* Both histograms need bug_numbers and alert_emails. alert_emails needs to have a real person who's responsible for the long-term value of the data (it may also have a list address).
* The description of A11Y_CONSUMERS mentions an enum but not it's name, but I can't figure out what the recorded values mean. If there is an existing in-tree enumeration, at least point to it (by name or DXR link), or if there's not, the description should include what each value means.

Non-blocking issues:
* for A11Y_INSTANTIATED_FLAG, flag histograms are basically deprecated now, and it would be better to use a boolean scalar
* both of these say record_in_processes main+content but I suspect that they are main-process-only and that would improve various aggregations
Attachment #8888518 - Flags: review?(benjamin) → review-

Comment 4

a year ago
Oh, and when you add bug_numbers and alert_emails you can remove these histograms from the whitelist of old histograms that don't follow the rules (histogram-whitelists.json)

Comment 5

a year ago
I forgot one non-blocking issue (can be a followup): typically opt-out metrics need to have a unit test to ensure that they still work. This is important because code refactorings fairly frequently break telemetry without people noticing. So I would strongly encourage you to try and get basic unit tests working for these metrics. I realize this might be difficult because our testing framework probably doesn't do much a11y.
(Assignee)

Updated

a year ago
Whiteboard: aes+
(Assignee)

Updated

a year ago
Blocks: 1384133
(Assignee)

Updated

a year ago
Blocks: 1384135
(Assignee)

Comment 6

a year ago
Created attachment 8889893 [details] [diff] [review]
patch

- added bug#s (this bug) to the two probes I'm converting to opt-out
- added emails for all four probes
- removed entries from white lists
- filed two follow ups, converting to boolean scalars and adding unit tests
Attachment #8888518 - Attachment is obsolete: true
Attachment #8889893 - Flags: review?(benjamin)

Comment 7

a year ago
Comment on attachment 8889893 [details] [diff] [review]
patch

data-r=me thanks!
Attachment #8889893 - Flags: review?(benjamin) → review+
(Assignee)

Comment 8

a year ago
Created attachment 8889894 [details] [diff] [review]
patch

- updating email address to something more generic
Attachment #8889893 - Attachment is obsolete: true
Attachment #8889894 - Flags: review?(benjamin)
(Assignee)

Comment 9

a year ago
Comment on attachment 8889894 [details] [diff] [review]
patch

+ from irc
Attachment #8889894 - Flags: review?(benjamin) → review+
(Assignee)

Comment 10

a year ago
Created attachment 8889896 [details] [diff] [review]
patch

s/been used/has been used text in comments.
Attachment #8889894 - Attachment is obsolete: true
Attachment #8889896 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 11

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/547d9dfbbf67
Convert a11y_consumers and a11y_instantiated_flag to opt-out. r=davidb, r=bsmedberg
Keywords: checkin-needed

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/547d9dfbbf67
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 13

a year ago
Comment on attachment 8889896 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]:
converting existing a11y telemetry probes to opt out
[User impact if declined]:
we'd like tyo have this data out on release for 55 so we can make some decisions about 56/57 rollout.
[Is this code covered by automated tests?]:
telemetry is
[Has the fix been verified in Nightly?]:
changed probe to opt out
[Needs manual test from QE? If yes, steps to reproduce]: 
no
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
existing well test probe converted to opt out on release
[String changes made/needed]:
none
Attachment #8889896 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a year ago
Blocks: 1384528
Comment on attachment 8889896 [details] [diff] [review]
patch

convert 2 a11y telemetry probes to opt-out, needed in 55 release.  should be in 55.0b13
Attachment #8889896 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e2dd499956f0
status-firefox55: --- → fixed
(In reply to Jim Mathies [:jimm] from comment #13)
> [Is this code covered by automated tests?]:
> telemetry is
> [Has the fix been verified in Nightly?]:
> changed probe to opt out
> [Needs manual test from QE? If yes, steps to reproduce]: 
> no

Setting qe-verify- based on Jim's assessment on manual testing needs.
Flags: qe-verify-
(Assignee)

Updated

a year ago
Target Milestone: mozilla56 → mozilla55
You need to log in before you can comment on or make changes to this bug.