Closed Bug 1363725 Opened 2 years ago Closed 2 years ago

Is it "all_child" or "all_childs"?

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: chutten, Assigned: hrdktg)

References

Details

Attachments

(1 file, 5 obsolete files)

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?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
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
> 
> ...
Flags: needinfo?(alessio.placitelli)
But should we "fix" the code to match the docs (and English grammar)?
As a non-native speaker i'll ask: Does "all_childs" sound correct?
If yes, let's just fix the docs.
Points: --- → 1
Flags: needinfo?(gfritzsche)
Priority: -- → P2
"all_childs" sounds incorrect. In English (ugh.) the plural of "child" is "children"
Priority: P2 → P3
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?
Flags: needinfo?(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)
Assignee: nobody → hrdktg
Status: NEW → ASSIGNED
Flags: needinfo?(chutten)
Changed all occurrence of 
>'all_child' to 'all_children'
>'all_childs' to 'all_children'
and
>'AllChilds' to 'AllChildren'
Attachment #8941186 - Flags: review?(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"
Attachment #8941186 - Flags: review?(chutten) → review-
(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 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!
Attachment #8941224 - Flags: review?(chutten) → review+
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?
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac990767e3e8
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Flags: needinfo?(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.
Flags: needinfo?(chutten)
Blocks: 1430115
Reopening to revert.
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
Attachment #8942208 - Flags: review?(alessio.placitelli) → review+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by chutten@mozilla.com:
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
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?
Flags: needinfo?(hrdktg)
Attached patch patch9.diff (obsolete) — Splinter Review
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"?
Attachment #8941224 - Attachment is obsolete: true
Flags: needinfo?(hrdktg)
Attachment #8947087 - Flags: review?(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 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.
Attachment #8947090 - Flags: review?(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.
Attachment #8947090 - Attachment is obsolete: true
Attachment #8947391 - Flags: review?(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.
Attachment #8947391 - Flags: review?(chutten) → review+
Attachment #8942208 - Attachment is obsolete: true
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/
Keywords: checkin-needed
Pushed by dluca@mozilla.com:
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
Keywords: checkin-needed
Flags: needinfo?(hrdktg)
https://hg.mozilla.org/mozilla-central/rev/df844dbc028e
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
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.
Duplicate of this bug: 1430115
Flags: needinfo?(hrdktg)
You need to log in before you can comment on or make changes to this bug.