Closed Bug 1288745 Opened 8 years ago Closed 8 years ago

Remove TelemetryHistogram::NewHistogram

Categories

(Toolkit :: Telemetry, defect, P3)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: chutten, Assigned: adamg2, Mentored)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Bug 1288745 - Remove TelemetryHistogram::NewHistogram and replace with predefined histograms.
58 bytes, text/x-review-board-request
chutten
: review+
Details 
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
Priority: -- → P3
Whiteboard: [measurement:client] [lang=js] [lang=c++]
Mentor: chutten
Assignee: nobody → adamgj.wong
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-
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.
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?
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 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+
Blocks: 1299143
Blocks: 1299144
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()`.
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 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 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+
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d28195b44f26
Remove TelemetryHistogram::NewHistogram and replace with predefined histograms. r=chutten
https://hg.mozilla.org/mozilla-central/rev/d28195b44f26
Status: NEW → RESOLVED
Closed: 8 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: