1188423 - Investigate and remove |*loadHistograms| methods from TelemetryStorage

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Alessio Placitelli [:Dexter]]

|TelemetryStorage.testLoadHistograms| and |TelemetryStorage.loadHistograms| don't seem to be used anymore in [1].

Moreover, TelemetryStorage.loadHistograms was populating the READ_SAVED_PING_SUCCESS histogram [2]: only |addPendingPing| is doing that at the moment, so the data in that histogram may not be trustworthy.

We should probably remove the loadHistograms* functions and the READ_SAVED_PING_SUCCESS histogram, since bug 1187340 is adding a new probe anyway.

[1] - https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/toolkit/components/telemetry/TelemetryStorage.jsm#l340
[2] - https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/toolkit/components/telemetry/TelemetryStorage.jsm#l1260

Comment 1

[Georg Fritzsche [:gfritzsche]]

TelemetryStorage.loadHistograms is not used anywhere.

The only use of testLoadHistograms is here:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#497

We can simply remove that test - it tests the loading and deletion of invalid JSON which is covered here too:
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetrySendOldPings.js#312

Comment 2

[Georg Fritzsche [:gfritzsche]]

Robert, would you be interested in taking this?

Comment 3

[rthyberg]

yeah sounds good

Comment 4

[Georg Fritzsche [:gfritzsche]]

While we're cleaning up those functions, addPendingPingFromFile() is unused too:
https://dxr.mozilla.org/mozilla-central/search?q=addPendingPingFromFile&redirect=false&case=true&limit=69&offset=0

That makes READ_SAVED_PING_SUCCESS definitely useless, so we should remove it from the histograms list too:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/Histograms.json#5257

Comment 5

[rthyberg]

Attachment: removed_loadHistograms

there were a few more assert() references to READ_SAVED_PING_SUCCESS that I ended up removing in test_TelemetrySession.js

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8662824 [details] [diff] [review]
removed_loadHistograms

Review of attachment 8662824 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -286,4 @@
>    const TELEMETRY_TEST_COUNT = "TELEMETRY_TEST_COUNT";
>    const TELEMETRY_TEST_KEYED_FLAG = "TELEMETRY_TEST_KEYED_FLAG";
>    const TELEMETRY_TEST_KEYED_COUNT = "TELEMETRY_TEST_KEYED_COUNT";
>    const READ_SAVED_PING_SUCCESS = "READ_SAVED_PING_SUCCESS";

We should remove this line too.

@@ -290,5 @@
>  
>    if (successfulPings > 0) {
>      Assert.ok(TELEMETRY_PING in payload.histograms);
>    }
> -  Assert.ok(READ_SAVED_PING_SUCCESS in payload.histograms);

There is one more location here that needs cleanup:
https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js#349

I recommend to run the unit tests locally to confirm this works properly:
mach xpcshell-test toolkit/components/telemetry/tests/unit/

Comment 7

[rthyberg]

Attachment: removed_loadHistograms

Yeah the test worked. I just forgot to refresh my patch before I uploaded it

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8662853 [details] [diff] [review]
removed_loadHistograms

Review of attachment 8662853 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b88ef597ae9f

Comment 10

[Carsten Book [:Tomcat]]

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