1376333 - Improve naming of accumulation types & variables

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Georg Fritzsche [:gfritzsche]]

As a follow-up to bug 1375043, we want to make the naming of the remaining "accumulation" names more clear:
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3ADiscardedAccumulations%7CDiscardedKeyedAccumulations%7CAccumulation%7CKeyedAccumulation+path%3Atoolkit%2Fcomponents%2Ftelemetry&redirect=false

Specifically, we want to rename these to HistogramAccumulations / KeyedHistogramAccumulations:
https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryComms.h#20,26

And these to histogramsToSend / keyedHistogramsToSend:
https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp#267-268

Comment 1

[Georg Fritzsche [:gfritzsche]]

Krishna, i think you wanted to take this bug per the comment in bug 1375043.

Comment 2

[Krishna]

Attachment: Bug 1376333 - Improve naming of accumulation types and variables. r=gfritzsche

Comment 3

[Krishna]

Georg, Please review the patch. Please suggest another bug on which may I work on. Thanks. :)

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8881409 [details] [diff] [review]
Bug 1376333 - Improve naming of accumulation types and variables. r=gfritzsche

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

Thanks!

Can you update the commit message to match our standard format?
It's "Bug xxx - Description text. r=gfritzsche"

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8881409 [details] [diff] [review]
Bug 1376333 - Improve naming of accumulation types and variables. r=gfritzsche

Hey, i tried to build this and it failed with a few errors.
Did you try building with this patch applied?

Comment 6

[Krishna]

Hello,
Sorry. I hadn't tried building after applying the patch. But I did the renaming you mentioned [1] and [2]. I don't understand what went wrong? Do I also need to rename [3]?
[1] https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryComms.h#20,26
[2] https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp#267-268
[3] https://dxr.mozilla.org/mozilla-central/search?q=regexp%3ADiscardedAccumulations%7CDiscardedKeyedAccumulations%7CAccumulation%7CKeyedAccumulation+path%3Atoolkit%2Fcomponents%2Ftelemetry&redirect=false

Comment 7

[Georg Fritzsche [:gfritzsche]]

When renaming the `Accumulation` & `KeyedÀccumulation` types, you also need to fix the other locations where these types are being used:
https://dxr.mozilla.org/mozilla-central/search?q=type%3AAccumulation&redirect=false
https://dxr.mozilla.org/mozilla-central/search?q=type%3AKeyedAccumulation&redirect=false

Comment 8

[Krishna]

Hello, Sorry I am late. I tried renaming as you suggested in the last comment. I still got a few errors like 'invalid use of incomplete type' and 'forward declaration'. I tried solving them by fixing types in other files as well, but to no avail. I am stumped as to how to proceed now.

Comment 9

[Nilabja Bhattacharya]

May I try to fix this issue? 
Moreover, changes should be made to these files right?

Specifically, we want to rename these to HistogramAccumulations / KeyedHistogramAccumulations:
https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryComms.h#20,26

And these to histogramsToSend / keyedHistogramsToSend:
https://dxr.mozilla.org/mozilla-central/rev/c01aa84ded7eb0b3e691f8bcc5cd887c960a779e/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp#267-268

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Nilabja Bhattacharya from comment #9)
> May I try to fix this issue? 

Nilabja, Krishna is still working on the bug. You're best bet is to pick another bug: have a look at https://www.joshmatthews.net/bugsahoy/ !

Comment 11

[Alessio Placitelli [:Dexter]]

(In reply to Krishna from comment #8)
> Hello, Sorry I am late. I tried renaming as you suggested in the last
> comment. I still got a few errors like 'invalid use of incomplete type' and
> 'forward declaration'. I tried solving them by fixing types in other files
> as well, but to no avail. I am stumped as to how to proceed now.

Krishna, are you still working on this bug? If you're stuck, would you mind attaching your latest version of the patch?

Comment 12

[Krishna]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> (In reply to Krishna from comment #8)
> > Hello, Sorry I am late. I tried renaming as you suggested in the last
> > comment. I still got a few errors like 'invalid use of incomplete type' and
> > 'forward declaration'. I tried solving them by fixing types in other files
> > as well, but to no avail. I am stumped as to how to proceed now.
> 
> Krishna, are you still working on this bug? If you're stuck, would you mind
> attaching your latest version of the patch?

Hello, I have tried renaming 'Accumulation' and 'KeyedAccumulation' types in these files [1] and [2]. Still, the build fails with errors. 
[1] https://dxr.mozilla.org/mozilla-central/search?q=type%3AAccumulation&redirect=false
[2] https://dxr.mozilla.org/mozilla-central/search?q=type%3AKeyedAccumulation&redirect=false

Comment 13

[Alessio Placitelli [:Dexter]]

(In reply to Krishna from comment #12)
> (In reply to Alessio Placitelli [:Dexter] from comment #11)
> > (In reply to Krishna from comment #8)
> > > Hello, Sorry I am late. I tried renaming as you suggested in the last
> > > comment. I still got a few errors like 'invalid use of incomplete type' and
> > > 'forward declaration'. I tried solving them by fixing types in other files
> > > as well, but to no avail. I am stumped as to how to proceed now.
> > 
> > Krishna, are you still working on this bug? If you're stuck, would you mind
> > attaching your latest version of the patch?
> 
> Hello, I have tried renaming 'Accumulation' and 'KeyedAccumulation' types in
> these files [1] and [2]. Still, the build fails with errors. 
> [1]
> https://dxr.mozilla.org/mozilla-central/
> search?q=type%3AAccumulation&redirect=false
> [2]
> https://dxr.mozilla.org/mozilla-central/
> search?q=type%3AKeyedAccumulation&redirect=false

The searches in comment 0 show up quite a few more files to change :) Have you tried changing all the files from these searches? If, after doing that, the build still fails, you can save the output of the build process to a text file, attach it to the bug and flag us for help there.

Comment 14

[Krishna]

Attachment: Build output after renaming in all files(mentioned)

Comment 15

[Alessio Placitelli [:Dexter]]

(In reply to Krishna from comment #14)
> Created attachment 8889842 [details]
> Build output after renaming in all files(mentioned)

It looks like you're not renaming all the occurrences. Could you please provide the patch that generates the log that you attached? Otherwise it's very hard for us to tell what's missing :)

Comment 16

[Krishna]

Attachment: Patch for Bug1376333 (FailedBuild)

Hello,
This is the patch that produces the given log and fails to build. I would be grateful if you could catch any mistakes I might have made. Thanks.

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8890205 [details] [diff] [review]
Patch for Bug1376333 (FailedBuild)

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

This is a good start. Other than fixing the problems below, you would also need to take care of renaming the entries mentioned in the previous comments to match the new names. To understand what needs to be changed, have a look at the search from comment 0: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3ADiscardedAccumulations%7CDiscardedKeyedAccumulations%7CAccumulation%7CKeyedAccumulation+path%3Atoolkit%2Fcomponents%2Ftelemetry&redirect=false

::: toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp
@@ +264,4 @@
>  SendAccumulatedData(TActor* ipcActor)
>  {
>    // Get the accumulated data and free the storage buffers.
> +  nsTArray<Accumulation> histogramsToSend;

You also need to reference the right type, since you renamed it in TelemetryComms.h

So this should be:
nsTArray<HistogramAccumulation> histogramsToSend;

@@ +264,5 @@
>  SendAccumulatedData(TActor* ipcActor)
>  {
>    // Get the accumulated data and free the storage buffers.
> +  nsTArray<Accumulation> histogramsToSend;
> +  nsTArray<KeyedAccumulation> keyedHistogramsToSend;

Please make sure to reference the correct type.

Comment 18

[Alessio Placitelli [:Dexter]]

Hey Krishna, are you still working on this?

Comment 19

[Krishna]

(In reply to Alessio Placitelli [:Dexter] from comment #18)
> Hey Krishna, are you still working on this?

Umm. No. You could assign the project to someone.

Comment 20

[F Seita [:fseita]]

Hi, I'd like to work on this bug, is it still open?

Comment 21

[Alessio Placitelli [:Dexter]]

(In reply to FSeita from comment #20)
> Hi, I'd like to work on this bug, is it still open?

Hi FSeita! Yes, it's still open. I've assigned it to you. See comment 0 for directions and let me know if you need further help!

Comment 22

[F Seita [:fseita]]

Attachment: Bug 1376333 - Improve naming of accumulation types & variables

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

Comment 23

[Alessio Placitelli [:Dexter]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review184478

Hey :fseita, thank you for your patch! It looks good, I'm just holding back the review because of the comment below. Could you please fix and submit an updated patch?

Moreover, did you check that the tests run locally on your machine? You can quickly test, after building, using ./mach test toolkit/components/telemetry

::: toolkit/components/telemetry/Telemetry.h:38
(Diff revision 1)
>    class HangAnnotations;
>  } // namespace HangMonitor
>  namespace Telemetry {
>  
> -struct Accumulation;
> -struct KeyedAccumulation;
> +struct HistogramAccumulation;
> +struct KeyedHistogramAccumulations;

Why did you change this to the plural form? Each `KeyedHistogramAccumulation` is a single accumulation for a single key.

Comment 24

[F Seita [:fseita]]

Attachment: Renamed mozilla::telemetry::KeyedHistogramAccumulations to singular KeyedHistogramAccumulation

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

Comment 25

[F Seita [:fseita]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review184478

I ran the tests, toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js failed both before and after applying my patch in the same way, so it should be unrelated.
Tested on Windows and Linux, got the same results.

Comment 26

[F Seita [:fseita]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review184478

> Why did you change this to the plural form? Each `KeyedHistogramAccumulation` is a single accumulation for a single key.

Oops sorry you're right, I can't believe I let a typo like that through.
What would be the preferred way to fix mistakes like that in the review process?
Creating a new commit and pushing it for review?
Rolling the fix into the old commit and pushing that?

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review184478

> Oops sorry you're right, I can't believe I let a typo like that through.
> What would be the preferred way to fix mistakes like that in the review process?
> Creating a new commit and pushing it for review?
> Rolling the fix into the old commit and pushing that?

No problem, don't worry :) Roll the fix into the old commit and push that ;)

Comment 28

[F Seita [:fseita]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

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

Comment 29

[Alessio Placitelli [:Dexter]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review185014

This looks good now, thanks! Can you push this to try? From the automation menu, trigger a try build. Let's pick linux, macos and windows and run xpcshell and gtests.

Comment 30

[F Seita [:fseita]]

Comment on attachment 8907566 [details]
Bug 1376333 - Improve naming of accumulation types & variables

https://reviewboard.mozilla.org/r/179272/#review185014

Hmm, I don't have access to the try server yet, I guess it's time to read more about that.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d196551463ab
Improve naming of accumulation types & variables r=Dexter

Comment 32

[Alessio Placitelli [:Dexter]]

Hey :fseita, since the tests look good, I took care of flagging this for landing. It should land in a day or so into mozilla-central :D Would you be interested in working on another bug? If so, what about bug 1362953?

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[F Seita [:fseita]]

(In reply to Alessio Placitelli [:Dexter] from comment #32)
> Would you be interested in working on another bug? If so, what about bug 1362953?

Ah deadlocks, my favourite type of bug.... I'll take a look!