Making the process choice explicit/required for Scalars

RESOLVED FIXED in Firefox 54

Status

()

Product:
Toolkit
Component:
Telemetry
Type:
defect
Importance:
P1
normal
Status:
RESOLVED FIXED
Opened:
3 years ago
Updated:
3 years ago

People

(Reporter: Dexter, Assigned: chutten)

Tracking

Version:
Trunk
Target:
mozilla54
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment)

bug 1333797 - Require 'record_in_processes' values in Scalars.yaml
59 bytes, text/x-review-board-request
Dexter
: review+
Details 
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.
Blocks: 1278556
Points: --- → 1
Priority: -- → P2
Whiteboard: [measurement:client]
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 chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/43c2c4c0324d
Require 'record_in_processes' values in Scalars.yaml r=Dexter
https://hg.mozilla.org/mozilla-central/rev/43c2c4c0324d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.