1299144 - Remove TelemetryHistogram::NewKeyedHistogram

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Chris H-C :chutten]

+++ 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

Comment 1

[Adam :adamg2]

Attachment: Bug 1299144 - Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms.

Review commit: https://reviewboard.mozilla.org/r/78106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/78106/

Comment 2

[Chris H-C :chutten]

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 3

[Adam :adamg2]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/78106/diff/1-2/

Comment 4

[Chris H-C :chutten]

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

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a38442afa931
Replace TelemetryHistogram::NewKeyedHistogram and replace with predefined histograms. r=chutten

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a38442afa931