1333797 - Making the process choice explicit/required for Scalars

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

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.

Comment 1

[Chris H-C :chutten]

Attachment: bug 1333797 - Require 'record_in_processes' values in Scalars.yaml

Having "main" be the default value is a very sensible choice, but one that is a
little opaque if you aren't into reading Telemetry code.

Just require a value here, and this will encourage correct use as it can be
caught and policed in data-review.

Review commit: https://reviewboard.mozilla.org/r/107198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107198/

Comment 2

[Alessio Placitelli [:Dexter]]

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 3

[Chris H-C :chutten]

Comment on attachment 8830443 [details]
bug 1333797 - Require 'record_in_processes' values in Scalars.yaml

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107198/diff/1-2/

Comment 4

[Alessio Placitelli [:Dexter]]

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!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43c2c4c0324d
Require 'record_in_processes' values in Scalars.yaml r=Dexter

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43c2c4c0324d