Closed
Bug 1341236
Opened 8 years ago
Closed 8 years ago
Drop the |dataset()| function from keyed & plain histograms
Categories
(Toolkit :: Telemetry, defect, P3)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: Dexter, Assigned: fionn_mac, Mentored)
References
Details
(Whiteboard: [measurement:client][lang=c++])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•8 years ago
|
Points: --- → 1
Priority: -- → P3
Whiteboard: [lang=c++]
Reporter | ||
Comment 1•8 years ago
|
||
Vedant, would you be interested in taking this bug?
Flags: needinfo?(vedant.sareen)
Assignee | ||
Comment 2•8 years ago
|
||
Sure Sir!
I'll start work on this ASAP :)
Flags: needinfo?(vedant.sareen)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → vedant.sareen
Whiteboard: [lang=c++] → [measurement:client][lang=c++]
Assignee | ||
Comment 3•8 years ago
|
||
(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
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 4•8 years ago
|
||
(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
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 5•8 years ago
|
||
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.
Attachment #8841730 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 6•8 years ago
|
||
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.
Attachment #8841730 -
Flags: review?(alessio.placitelli) → feedback+
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8841730 -
Attachment is obsolete: true
Attachment #8842119 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 8•8 years ago
|
||
(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.
Reporter | ||
Comment 9•8 years ago
|
||
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..."
Attachment #8842119 -
Flags: review?(alessio.placitelli) → feedback+
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8842119 -
Attachment is obsolete: true
Attachment #8843965 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 11•8 years ago
|
||
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!
Attachment #8843965 -
Flags: review?(alessio.placitelli) → review+
Reporter | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
(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?
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c69f0dceee
Drop the |dataset()| function from keyed & plain histograms. r=Dexter
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•