The default bug view has changed. See this FAQ.

Remove TelemetryHistogram::NewHistogram

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: chutten, Assigned: adamg2, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [measurement:client] [lang=js] [lang=c++])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 months ago
NewHistogram is only used by tests [1]. For tests we already have the TELEMETRY_TEST_* histograms if we need histograms of particular compositions.

1) Add necessary TELEMETRY_TEST_* histograms
2) Convert tests to use TELEMETRY_TEST_* histograms instead of NewHistogram
3) Remove NewHistogram and anything else that supports only it.

[1]: https://dxr.mozilla.org/mozilla-central/search?q=ewHistogram&case=true&=mozilla-central
Mentor: gfritzsche@mozilla.com
Priority: -- → P3
Whiteboard: [measurement:client] [lang=js] [lang=c++]
Mentor: chutten@mozilla.com
Assignee: nobody → adamgj.wong
Comment hidden (mozreview-request)
(Reporter)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review72790

Looking good, just a few small things and then it'll be good to go.

::: toolkit/components/telemetry/Histograms.json:5194
(Diff revision 1)
> +    "expires_in_version" : "never",
> +    "kind": "boolean",
> +    "bug_numbers": [1288745],
> +    "description": "a testing histogram; not meant to be touched"
> +  },
> +  "TELEMETRY_TEST_FOOBAR": {

Should this not be TELEMETRY_TEST_FLAG to match the previous? Or, perhaps, TELEMETRY_TEST_EXPIRED?

::: toolkit/components/telemetry/Telemetry.cpp:1032
(Diff revision 1)
> -                                          min, max, bucketCount,
> -                                          cx, optArgCount, ret);
> -}
> -
> -NS_IMETHODIMP
>  TelemetryImpl::NewKeyedHistogram(const nsACString &name, const nsACString &expiration, uint32_t histogramType,

Did you happen to notice whether NewKeyedHistogram was being used outside of tests while you were in here? It might be a decent follow-up bug to remove it, if so.

::: toolkit/components/telemetry/histogram-whitelists.json:935
(Diff revision 1)
>      "TELEMETRY_PING",
>      "TELEMETRY_SEND",
>      "TELEMETRY_STRINGIFY",
>      "TELEMETRY_SUCCESS",
>      "TELEMETRY_TEST_COUNT",
> +    "TELEMETRY_TEST_COUNT2",

Do not add new test histograms to the whitelists. Instead, make them conform to the new standard. (has bug_numbers, has alert_emails, has n_buckets)

::: toolkit/components/telemetry/nsITelemetry.idl:65
(Diff revision 1)
>     *   histogram_type - HISTOGRAM_EXPONENTIAL, HISTOGRAM_LINEAR, HISTOGRAM_BOOLEAN
>     *                    or HISTOGRAM_COUNT
>     *   counts - array representing contents of the buckets in the histogram
>     *   ranges -  an array with calculated bucket sizes
>     *   sum - sum of the bucket contents
>     *   static - true for histograms defined in TelemetryHistograms.h, false for ones defined with newHistogram

Hm. File a follow-up for the removal of snapshot.static? Or would it make more sense to remove it under this bug...

::: toolkit/components/telemetry/tests/unit/test_TelemetryStopwatch.js:10
(Diff revision 1)
>  Cu.import("resource://gre/modules/TelemetryStopwatch.jsm", tmpScope);
>  var TelemetryStopwatch = tmpScope.TelemetryStopwatch;
>  
>  // We can't create a histogram here since the ones created with
>  // newHistogram are not seen by getHistogramById that the module uses.
> +// However becaue of bug_number 1288745, newHistogram is now deprecated and removed.

I'm in favour of dropping the comment rather than adding the amendment about bug 1288745
Attachment #8785766 - Flags: review?(chutten) → review-
(Reporter)

Comment 3

7 months ago
Oh, and about the commit message: you can remove the word "number". You may also wish to write it in the present tense (Remove instead of Removed) to match the style of other commit summaries.
(Assignee)

Comment 4

7 months ago
mozreview-review-reply
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review72790

> Should this not be TELEMETRY_TEST_FLAG to match the previous? Or, perhaps, TELEMETRY_TEST_EXPIRED?

TELEMETRY_TEST_FOOBAR was used as a replacement in 2 locations where the histogram_id was set to "FOOBAR", and the test for TELEMETRY_TEST_EXPIRED was being run in addition to the FOOBAR'd one (test_TelemetrySession.js:492).  Should I rename this to something else?
(Reporter)

Comment 5

7 months ago
mozreview-review-reply
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review72790

> TELEMETRY_TEST_FOOBAR was used as a replacement in 2 locations where the histogram_id was set to "FOOBAR", and the test for TELEMETRY_TEST_EXPIRED was being run in addition to the FOOBAR'd one (test_TelemetrySession.js:492).  Should I rename this to something else?

In both cases it is testing the behaviour of expired histograms (that anything you do to them or their clones doesn't actually matter), so its name might most helpfully be "TELEMETRY_TEST_EXPIRED"
Comment hidden (mozreview-request)
(Reporter)

Comment 7

7 months ago
mozreview-review
Comment on attachment 8786226 [details]
Bug 1288745 - Remove TELEMETRY_TEST_FOOBAR, clean up comments.

https://reviewboard.mozilla.org/r/75222/#review73132

These changes are good. Still a couple of outstanding things on the original commit, then squash (fixing the message) and I think we can get this in line to be checked in.
Attachment #8786226 - Flags: review?(chutten) → review+
(Reporter)

Updated

7 months ago
Blocks: 1299143
(Reporter)

Updated

7 months ago
Blocks: 1299144
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8786226 - Attachment is obsolete: true
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review73520

::: toolkit/components/telemetry/nsITelemetry.idl
(Diff revision 2)
> -   * The returned object has the following functions:
> -   *   add(int) - Adds an int value to the appropriate bucket
> -   *   snapshot() - Returns a snapshot of the histogram with the same data fields as in histogramSnapshots()
> -   *   clear() - Zeros out the histogram's buckets and sum
> -   *   dataset() - identifies what dataset this is in: DATASET_RELEASE_CHANNEL_OPTOUT or ...OPTIN

This is the only comment documenting the returned object, the comments for `getHistogramById()` & `histogramFrom()` refer to it.
We should move this documentation to the comment for `getHistogramById()`.
(Reporter)

Comment 10

7 months ago
mozreview-review
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review74394
Attachment #8785766 - Flags: review?(chutten) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 12

7 months ago
mozreview-review-reply
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review73520

> This is the only comment documenting the returned object, the comments for `getHistogramById()` & `histogramFrom()` refer to it.
> We should move this documentation to the comment for `getHistogramById()`.

Added them to both getHistogramById() and histogramFrom() for clarity.  Should it only be one copy of the comment?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1201022
Blocks: 1300491
(Reporter)

Comment 15

7 months ago
mozreview-review
Comment on attachment 8785766 [details]
Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/74848/#review75036

Everything looks groovy to me!
Attachment #8785766 - Flags: review?(chutten) → review+

Comment 16

7 months ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d28195b44f26
Remove TelemetryHistogram::NewHistogram and replace with predefined histograms. r=chutten

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d28195b44f26
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.