Closed Bug 1333797 Opened 5 years ago Closed 5 years ago
Making the process choice explicit/required for Scalars
59 bytes, text/x-review-board-request
In bug 1278556 comments 1 (and 3) we explain why it's important to restrict scalar collection to specific processes. However, as shown by bug 1325536, making the "main" process the default if no 'record_in_processes' option is specified could be confusing. We should make the process explicit or, even better, make the 'record_in_processes' property in Scalars.yaml mandatory for each definition.
Points: --- → 1
Priority: -- → P2
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8830443 [details] bug 1333797 - Require 'record_in_processes' values in Scalars.yaml https://reviewboard.mozilla.org/r/107198/#review108656 Hey Chris, thanks for picking this up. I was waiting for the discussion to settle on FHR-dev before reviewing this. It looks ok with the below addressed. Please also update the docs (scalars.rst). ::: toolkit/components/telemetry/parse_scalars.py:232 (Diff revision 1) > return self._definition['notification_emails'] > > @property > def record_in_processes(self): > """Get the non-empty list of processes to record data in""" > return self._definition.get('record_in_processes', ['main']) Let's drop the default value from here since we're requiring the field now.
Comment on attachment 8830443 [details] bug 1333797 - Require 'record_in_processes' values in Scalars.yaml https://reviewboard.mozilla.org/r/107198/#review109618 Thanks Chris, this looks good!
Attachment #8830443 - Flags: review?(alessio.placitelli) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/43c2c4c0324d Require 'record_in_processes' values in Scalars.yaml r=Dexter
You need to log in before you can comment on or make changes to this bug.