1341236 - Drop the |dataset()| function from keyed & plain histograms

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

When getting an histogram using the getHistogramById function (or the keyed counterpart), the returned object exposes the |dataset| function as well, together with other functions [1].

We should just drop the dataset() functions from keyed & plain histograms and test via the nsITelemetry snapshot functions instead.

[1] - http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/toolkit/components/telemetry/TelemetryHistogram.cpp#1619,1909

Comment 1

[Alessio Placitelli [:Dexter]]

Vedant, would you be interested in taking this bug?

Comment 2

[Vedant Sareen [:fionn_mac]]

Sure Sir!

I'll start work on this ASAP :)

Comment 3

[Vedant Sareen [:fionn_mac]]

(In reply to Alessio Placitelli [:Dexter] from comment #0)
> When getting an histogram using the getHistogramById function (or the keyed
> counterpart), the returned object exposes the |dataset| function as well,
> together with other functions [1].
> 
> We should just drop the dataset() functions from keyed & plain histograms

I removed the mentioned lines from the linked file, and then ran mochitest and xpcshell-test to quickly find out the tests whose results might get affected due to these changes.

Sure enough, there was an error at http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js#777

Since we're dropping dataset() functions from these Histograms, the first half of this test will no longer work (all other tests finished successfully), so some change is definitely required here.

> and test via the nsITelemetry snapshot functions instead.

I didn't get what you meant by that though. I did go through the page linked below, but I didn't get how to check whether datasets work as expected using nsITelemetry snapshot functions, as it simply returns an object containing a snapshot from all of the currently registered histograms.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITelemetry

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Vedant Sareen [:fionn_mac] from comment #3)
> (In reply to Alessio Placitelli [:Dexter] from comment #0)
> > When getting an histogram using the getHistogramById function (or the keyed
> > counterpart), the returned object exposes the |dataset| function as well,
> > together with other functions [1].
> > 
> > We should just drop the dataset() functions from keyed & plain histograms
> 
> I removed the mentioned lines from the linked file, and then ran mochitest
> and xpcshell-test to quickly find out the tests whose results might get
> affected due to these changes.
> 
> Sure enough, there was an error at
> http://searchfox.org/mozilla-central/rev/
> 12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/toolkit/components/telemetry/tests/
> unit/test_TelemetryHistograms.js#777
> 
> Since we're dropping dataset() functions from these Histograms, the first
> half of this test will no longer work (all other tests finished
> successfully), so some change is definitely required here.

I think we can drop these lines, as the same tests are performed a few lines below [1].
The |dataset()| tests are testing the same behaviour with the API you're removing.

> > and test via the nsITelemetry snapshot functions instead.
> 
> I didn't get what you meant by that though. I did go through the page linked
> below, but I didn't get how to check whether datasets work as expected using
> nsITelemetry snapshot functions, as it simply returns an object containing a
> snapshot from all of the currently registered histograms.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsITelemetry

Ouch, that page seems to be very outdated! Sorry about that. I meant using |registeredHistograms| and |snapshotSubsessionHistogram| but the code at [1] is already doing that, so we should be safe in removing that part of the test.

[1] - http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js#783-802

Comment 5

[Vedant Sareen [:fionn_mac]]

Attachment: Proposed patch for Bug 1341236

I was able to build successfully.
I also ran |./mach xpcshell-test toolkit/components/telemetry/| and |./mach mochitest toolkit/components/telemetry/|. The patch passed all tests.

Comment 6

[Alessio Placitelli [:Dexter]]

Comment on attachment 8841730 [details] [diff] [review]
Proposed patch for Bug 1341236

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

This looks good overall. We're no longer using the implementation for the dataset function, so we must remove them too.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -1619,5 @@
>          && JS_DefineFunction(cx, obj, "snapshot",
>                               internal_JSHistogram_Snapshot, 0, 0)
> -        && JS_DefineFunction(cx, obj, "clear", internal_JSHistogram_Clear, 0, 0)
> -        && JS_DefineFunction(cx, obj, "dataset",
> -                             internal_JSHistogram_Dataset, 0, 0))) {

Is internal_JSHistogram_Dataset still being used? If not, let's remove it.

@@ -1915,5 @@
>                               internal_JSKeyedHistogram_Keys, 0, 0)
>          && JS_DefineFunction(cx, obj, "clear",
> -                             internal_JSKeyedHistogram_Clear, 0, 0)
> -        && JS_DefineFunction(cx, obj, "dataset",
> -                             internal_JSKeyedHistogram_Dataset, 0, 0))) {

Is internal_JSKeyedHistogram_Dataset still being used? If not, let's remove it.

Comment 7

[Vedant Sareen [:fionn_mac]]

Attachment: Rectified patch for Bug 1341236

Comment 8

[Vedant Sareen [:fionn_mac]]

(In reply to Alessio Placitelli [:Dexter] from comment #6)
> Comment on attachment 8841730 [details] [diff] [review]
> Proposed patch for Bug 1341236
> 
> Review of attachment 8841730 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good overall. We're no longer using the implementation for the
> dataset function, so we must remove them too.
> 
> ::: toolkit/components/telemetry/TelemetryHistogram.cpp
> @@ -1619,5 @@
> >          && JS_DefineFunction(cx, obj, "snapshot",
> >                               internal_JSHistogram_Snapshot, 0, 0)
> > -        && JS_DefineFunction(cx, obj, "clear", internal_JSHistogram_Clear, 0, 0)
> > -        && JS_DefineFunction(cx, obj, "dataset",
> > -                             internal_JSHistogram_Dataset, 0, 0))) {
> 
> Is internal_JSHistogram_Dataset still being used? If not, let's remove it.
> 
> @@ -1915,5 @@
> >                               internal_JSKeyedHistogram_Keys, 0, 0)
> >          && JS_DefineFunction(cx, obj, "clear",
> > -                             internal_JSKeyedHistogram_Clear, 0, 0)
> > -        && JS_DefineFunction(cx, obj, "dataset",
> > -                             internal_JSKeyedHistogram_Dataset, 0, 0))) {
> 
> Is internal_JSKeyedHistogram_Dataset still being used? If not, let's remove
> it.

Oops. Should've noticed that!
I've uploaded the patch with appropriate changes.

Comment 9

[Alessio Placitelli [:Dexter]]

Comment on attachment 8842119 [details] [diff] [review]
Rectified patch for Bug 1341236

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

Thanks Vedant! Sorry, for not catching this earlier, but there are a couple of comments left to fix. Once that's done, the patch is good to go.

Please make sure that everything builds and all the tests pass locally.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1597,3 @@
>    if (!(JS_DefineFunction(cx, obj, "add", internal_JSHistogram_Add, 1, 0)
>          && JS_DefineFunction(cx, obj, "snapshot",
>                               internal_JSHistogram_Snapshot, 0, 0)

nit: please update the comment a few lines above. Change "The 4 functions that..." to "The 3 functions that..."

@@ +1866,4 @@
>          && JS_DefineFunction(cx, obj, "keys",
>                               internal_JSKeyedHistogram_Keys, 0, 0)
>          && JS_DefineFunction(cx, obj, "clear",
> +                             internal_JSKeyedHistogram_Clear, 0, 0))) {

nit: please change the comment a few lines above from "The 7 functions that are wrapped..." to "The 6 functions that are wrapped..."

Comment 10

[Vedant Sareen [:fionn_mac]]

Attachment: Patch for Bug 1341236

Patch with changes in the comments regarding the wrapped up functions.

I was able to build successfully, and the patch passed the local xpcshell-test and mochitest (limited to tests in the directory |toolkit/components/telemetry|).

Apologies for my absent-mindedness.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8843965 [details] [diff] [review]
Patch for Bug 1341236

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

No problem! Thank you for your patch Vedant, this looks good now!

Comment 12

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2712d63cd2dd

Comment 13

[Vedant Sareen [:fionn_mac]]

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> Comment on attachment 8843965 [details] [diff] [review]
> Patch for Bug 1341236
> 
> Review of attachment 8843965 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> No problem! Thank you for your patch Vedant, this looks good now!

That's good to know! :)
Is there any other bug under you that I can work on?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c69f0dceee
Drop the |dataset()| function from keyed & plain histograms. r=Dexter

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/74c69f0dceee