1363725 - Is it "all_child" or "all_childs"?

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Chris H-C :chutten]

The docs say that a valid value for "record_in_processes" is "all_child", but the code requires "all_childs" (and the enum AllChilds).

Which should it be?

Comment 1

[Alessio Placitelli [:Dexter]]

Given bug 1278556 comment 3, I think the doc is wrong and the code is correct.

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> ...
> * possibly support "all" or "all childs" to not make measurements that run
> everywhere too complicated
> 
> ...

Comment 2

[Chris H-C :chutten]

But should we "fix" the code to match the docs (and English grammar)?

Comment 3

[Georg Fritzsche [:gfritzsche]]

As a non-native speaker i'll ask: Does "all_childs" sound correct?
If yes, let's just fix the docs.

Comment 4

[Chris H-C :chutten]

"all_childs" sounds incorrect. In English (ugh.) the plural of "child" is "children"

Comment 5

[hardik tyagi [:hrdktg]]

I would be happy to work on this bug.

I think that 'all_childs' is certainly not correct and should be changed to "all_children".
So should I change the code?

Comment 6

[Chris H-C :chutten]

The code, tests, and documentation will all need to be changed, yes. Here's a quick search showing our uses of "all_childs": https://searchfox.org/mozilla-central/search?q=all_childs&path=

And here's a search for "all_child" (excluding all_children): https://searchfox.org/mozilla-central/search?q=all_child%5B%5E%5Cw%5D&case=false&regexp=true&path=

Please let me know if you have any further questions (though note after today I may not be able to get back to you until 2018 due to end-of-year holidays)

Comment 7

[hardik tyagi [:hrdktg]]

Attachment: Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Changed all occurrence of 
>'all_child' to 'all_children'
>'all_childs' to 'all_children'
and
>'AllChilds' to 'AllChildren'

Comment 8

[Chris H-C :chutten]

Comment on attachment 8941186 [details] [diff] [review]
Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Review of attachment 8941186 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

I have a few small notes that we can take care of while we're in the area, and then this will be good to go.

::: toolkit/components/telemetry/TelemetryCommon.cpp
@@ +70,4 @@
>  bool
>  CanRecordInProcess(RecordedProcessType processes, GeckoProcessType processType)
>  {
> +  bool recordAllChild = !!(processes & RecordedProcessType::AllChildren);

While we're here, can we change this bool's name to be recordAllChildren as well?

::: toolkit/components/telemetry/TelemetryCommon.h
@@ +21,2 @@
>    Main       = (1 << GeckoProcessType_Default),  // Also known as "parent process"
>    Content    = (1 << GeckoProcessType_Content),

Hmm, maybe we'd better align all of the "=" again.

@@ +21,4 @@
>    Main       = (1 << GeckoProcessType_Default),  // Also known as "parent process"
>    Content    = (1 << GeckoProcessType_Content),
>    Gpu        = (1 << GeckoProcessType_GPU),
> +  AllChildren  = 0xFFFFFFFF - 1,  // All the children processes (i.e. content, gpu, ...)

While we're here, the comment should be "child" instead of "children"

Comment 9

[hardik tyagi [:hrdktg]]

Attachment: Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

All Done :)

***
BTW there are some chunks of comments that use "children processes".
https://searchfox.org/mozilla-central/search?q=children+Processes&case=false&regexp=true&path=

Comment 10

[Chris H-C :chutten]

(In reply to hardik tyagi [:hrdktg] from comment #9)
> BTW there are some chunks of comments that use "children processes".
> https://searchfox.org/mozilla-central/
> search?q=children+Processes&case=false&regexp=true&path=

They're in parts of the code far enough away that we can't take care of them in this patch (it'd be weird). But you can file bugs in those components to fix them, if you'd like! Toolkit::Performance Monitoring and Core::IPC ought to be the correct places to file and fix those bugs.

Comment 11

[Chris H-C :chutten]

Comment on attachment 8941224 [details] [diff] [review]
Bug 1363725 - Changed inconsistent occurrence of 'all_child',  'all_childs'  to  'all_children'  and  'AllChilds'  to 'AllChildren'  in code and docs.  r=chutten

Review of attachment 8941224 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent cleanup, thank you!

Comment 12

[Chris H-C :chutten]

I have marked this for checkin. Thank you for your contribution! Are you going to go ahead with filing and fixing those new cleanup bugs, or do you have particular ideas for what you'd like to work on next?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac990767e3e8
Change inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ac990767e3e8

Comment 15

[Alessio Placitelli [:Dexter]]

Hey, does this require any update upstream for consumers using parse_*.py scripts or shared_telemetry_utils.py?
The probe-scraper, for example, will fail because of this change:

>  from: {'node': u'7aa26468189e3e3f52d7cbd149297aed8191f16d', 'version': '59'}
> Traceback (most recent call last):
>   File "probe_scraper/runner.py", line 106, in <module>
>     main(args.tempdir, args.outdir, args.break_by_channel)
>   File "probe_scraper/runner.py", line 62, in main
>     results = PARSERS[probe_type].parse(paths, details["version"])
>   File "/mnt/tmp/probe-scraper/probe_scraper/parsers/scalars.py", line 31, in parse
>     scalars = parse_scalars.load_scalars(filenames[0])
>   File "/mnt/tmp/probe-scraper/probe_scraper/parsers/third_party/parse_scalars.py", line 288, in load_scalars
>     scalar_list.append(ScalarType(group_name, probe_name, scalar_info))
>   File "/mnt/tmp/probe-scraper/probe_scraper/parsers/third_party/parse_scalars.py", line 39, in __init__
>     self.validate_values(definition)
>   File "/mnt/tmp/probe-scraper/probe_scraper/parsers/third_party/parse_scalars.py", line 172, in validate_values
>     raise ValueError(self._name + ' - unknown value in record_in_processes: ' + proc)
> ValueError: keyed_accumulations - unknown value in record_in_processes: all_children

Comment 16

[Chris H-C :chutten]

Ohhhhhhhh poop. You're right.

This requires an update upstream for consumers of Histograms.json, Scalars.yaml, and Events.yaml since the format changed.

Comment 17

[Chris H-C :chutten]

Reopening to revert.

Comment 18

[Chris H-C :chutten]

Attachment: Revert "Bug 1363725 - Change inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs."

This reverts commit c238437964d1bfedd389ae72fd0da0f5f119211a.

It caused upstream processing headaches. We'll need to address those before
relanding.

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

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8942208 [details]
Revert "Bug 1363725 - Change inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs."

https://reviewboard.mozilla.org/r/212478/#review218202

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f345451c2083
Revert "Bug 1363725 - Change inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs." r=Dexter

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f345451c2083

Comment 22

[Chris H-C :chutten]

Sorry for the delay getting back to this, :hrdktg. It's a more complicated issue than I thought it would be.

Could you adjust your patch so that it still accepts "all_childs" (in a similar way to how I hotpatched it in bug 1430115)? When that's finished, we can coordinate with the owners of the mozilla/moztelemetry and mozilla/probe-scraper repositories to make sure they update their parser code at the same time we update the .json and .yaml files?

Comment 23

[hardik tyagi [:hrdktg]]

Attachment: patch9.diff

Apologies for the inactivity from my side.
I was exploring 'probe-scraper'.

This stuff is a little bit confusing.

This is what I understand right now->
1) probe-scraper has its own 'shared_telemetry_utils.py', which is weird.
2) 'shared_telemetry_utils.py' is used to validate values of 'record_in_process'.
3) Changing .json and .yaml on our side would conflict with probe-scraper working, So we cant change them.


> Could you adjust your patch so that it still accepts "all_childs" (in a
> similar way to how I hotpatched it in bug 1430115)? When that's finished, we
> can coordinate with the owners of the mozilla/moztelemetry and
> mozilla/probe-scraper repositories to make sure they update their parser
> code at the same time we update the .json and .yaml files?
Done.
I have left the .json and .yaml files with 'all_childs'.

Also,
Should i open up an issue on github(https://github.com/mozilla/probe-scraper)
and create a patch for that 'shared_telemetry_utils.py' as well?
After the commit on both sides we can update the .json and .yaml files here.

And I have left the docs part..What should be mentioned there for all_childs, something like "Depreciated, Use all_children instead"?

Comment 24

[hardik tyagi [:hrdktg]]

Attachment: Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Added Commit message.

Comment 25

[Chris H-C :chutten]

(In reply to hardik tyagi [:hrdktg] from comment #23)
> Created attachment 8947087 [details] [diff] [review]
> patch9.diff
> 
> Apologies for the inactivity from my side.
> I was exploring 'probe-scraper'.
> 
> This stuff is a little bit confusing.
> 
> This is what I understand right now->
> 1) probe-scraper has its own 'shared_telemetry_utils.py', which is weird.
> 2) 'shared_telemetry_utils.py' is used to validate values of
> 'record_in_process'.
> 3) Changing .json and .yaml on our side would conflict with probe-scraper
> working, So we cant change them.


Your three points are broadly correct.

1) probe-scraper actually "vendors" (copies) mozilla-central's shared_telemetry_utils.py and other parsers from time-to-time. So does mozilla/python_moztelemetry[1]. I believe they are the only two who do it.

2) Definitely correct. shared_telemetry_utils.py is used by the parse_*.py parsers to determine what is acceptable and what is not.

3) We _can_ change .json and .yaml on our side so long as we only change it in backwards-compatible ways (for example, bug 1422849). If we want to change it in a non-backwards-compatible way (like here, when we're changing to a new, previously-not-allowed value) we _still_ can change it... but we need to ensure we update probe-scraper and python_moztelemetry at roughly the same time. Or, as you suggest, by adopting a two-step approach.

[1]: https://github.com/mozilla/python_moztelemetry/tree/master/moztelemetry
 
> > Could you adjust your patch so that it still accepts "all_childs" (in a
> > similar way to how I hotpatched it in bug 1430115)? When that's finished, we
> > can coordinate with the owners of the mozilla/moztelemetry and
> > mozilla/probe-scraper repositories to make sure they update their parser
> > code at the same time we update the .json and .yaml files?
> Done.
> I have left the .json and .yaml files with 'all_childs'.

This is very sensible, and a good idea. Please file a follow-up bug to change all_childs to all_children after we update probe-scraper and python_moztelemetry. To that end, please open issues on probe-scraper and python_moztelemetry for the needed updates.

> And I have left the docs part..What should be mentioned there for
> all_childs, something like "Depreciated, Use all_children instead"?

Nope! When we're done we'll only support all_childs for the parsing of historical definitions files on the server. We can just remove all_childs in favour of all_children.

Comment 26

[Chris H-C :chutten]

Comment on attachment 8947090 [details] [diff] [review]
Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Review of attachment 8947090 [details] [diff] [review]:
-----------------------------------------------------------------

Please also include the docs change as mentioned above.

::: toolkit/components/telemetry/shared_telemetry_utils.py
@@ +19,5 @@
>      'main': 'Main',
>      'content': 'Content',
>      'gpu': 'Gpu',
> +    #Historical Values
> +    'all_childs': 'AllChildren', #Supporting files from before bug 1363725

You should double-check with ./mach lint, but I think you need a space between # and the comment to adhere to the style guide.

Comment 27

[hardik tyagi [:hrdktg]]

Attachment: Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

>Please also include the docs change as mentioned above.
Done.

>You should double-check with ./mach lint, 
>but I think you need a space between # and 
>the comment to adhere to the style guide.
Done.

Comment 28

[Chris H-C :chutten]

Comment on attachment 8947391 [details] [diff] [review]
Bug 1363725 - Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Review of attachment 8947391 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 29

[Chris H-C :chutten]

Let's get this in.

While we're letting that happen, :hrdktg would you mind filing Issues against probe-scraper[1] and python_moztelemetry[2] to have them update their parser scripts? Reference this bug in the Issues, and if you'd like to lend a hand making it happen, ask for help on how to do it properly (one time I missed a file when updating. No permanent harm done, but I felt bad).

[1]: https://github.com/mozilla/probe-scraper/
[2]: https://github.com/mozilla/python_moztelemetry/

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/df844dbc028e
Changed inconsistent occurrence of 'all_child', 'all_childs' to 'all_children' and 'AllChilds' to 'AllChildren' in code and docs. r=chutten

Comment 31

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/df844dbc028e

Comment 32

[Chris H-C :chutten]

Probe scraper issue[1] and PR[2].

Let me know if you run across any problems with probe-scraper or python_moztelemetry.

[1]: https://github.com/mozilla/probe-scraper/issues/33
[2]: https://github.com/mozilla/probe-scraper/pull/34

Comment 33

[Chris H-C :chutten]

Actually, :gfritzsche made a good point recently that this might be a good time to wean python_moztelemetry off of copying the parsers around and convert it over to use the Probe Info Service directly.

I have filed https://github.com/mozilla/python_moztelemetry/issues/193 to track updating python_moztelemetry.