The default bug view has changed. See this FAQ.

Remove TelemetryHistogram::NewKeyedHistogram

RESOLVED FIXED in Firefox 51

Status

()

Toolkit
Telemetry
P3
normal
RESOLVED FIXED
7 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)

(Reporter)

Description

7 months ago
+++ This bug was initially created as a clone of Bug #1288745 +++

NewKeyedHistogram 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 NewKeyedHistogram
3) Remove NewKeyedHistogram and anything else that supports only it (for instance, I think IsValidHistogramName would now no longer be used)

[1]: https://dxr.mozilla.org/mozilla-central/search?q=ewKeyedHistogram&case=true&=mozilla-central
Assignee: adamgj.wong → nobody
Status: ASSIGNED → NEW
Assignee: nobody → adamgj.wong
Blocks: 1201022
Comment hidden (mozreview-request)
(Reporter)

Comment 2

7 months ago
mozreview-review
Comment on attachment 8790153 [details]
Bug 1299144 - Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/78106/#review76628

::: toolkit/components/telemetry/nsITelemetry.idl:231
(Diff revision 1)
>    /**
> -   * Same as newKeyedHistogram above, but for histograms registered in TelemetryHistograms.h.
> +   * Create and return a histogram registered in TelemetryHistograms.h.
>     *
>     * @param id - unique identifier from TelemetryHistograms.h
> -   * The returned object has the same functions as a histogram returned from newKeyedHistogram.
> +   * The returned object has the following functions:
> +   *   add(string key, [optional] int) - Add an int value to the histogram for that key. If no histogram for that key exists yet, it is created.

Nice catch

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js:629
(Diff revision 1)
>    h.add(TEST_KEY, 1);
>    Assert.equal(h.snapshot(TEST_KEY).sum, 0,
>                 "The keyed histograms should not record any data.");
>  
>    // Runtime created histograms should not be recorded.
> -  h = Telemetry.newKeyedHistogram("test::runtime_keyed_boolean", "never", Telemetry.HISTOGRAM_BOOLEAN);
> +  h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_BOOLEAN");

This test becomes a clone of the above "Extended set keyed histograms should not be recorded", so remove it instead.
Comment hidden (mozreview-request)
(Reporter)

Comment 4

7 months ago
mozreview-review
Comment on attachment 8790153 [details]
Bug 1299144 - Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms.

https://reviewboard.mozilla.org/r/78106/#review77044
Attachment #8790153 - Flags: review?(chutten) → review+

Comment 5

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

Comment 6

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a38442afa931
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.