1379125 - Support recording discarded pings with Health Ping in all available places.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Kate Ustiuzhanina]

Locations: https://dxr.mozilla.org/mozilla-central/search?q=TELEMETRY_PING_SIZE_EXCEEDED_PENDING+path%3ATelemetryStorage.jsm&redirect=false

Comment 1

[Kate Ustiuzhanina]

Attachment: discardedPending.patch

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8890894 [details] [diff] [review]
discardedPending.patch

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +1368,5 @@
>        Telemetry.getHistogramById("TELEMETRY_PING_SIZE_EXCEEDED_PENDING").add();
>        TelemetryStopwatch.cancel("TELEMETRY_PENDING_LOAD_MS");
> +
> +      // String param must be changed to ping.type after resolving Bug 1384903
> +      TelemetryHealthPing.recordDiscardedPing("<unknown>");

We should document this special "<unknown>" value in health-ping.rst.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHealthPing.js
@@ +228,5 @@
> +  await TelemetryStorage.savePendingPing(OVERSIZED_PING);
> +
> +  // Try to manually load the oversized ping.
> +  await Assert.rejects(TelemetryStorage.loadPendingPing(OVERSIZED_PING_ID),
> +    "The oversized ping should have been pruned.");

This only tests the one code path through `loadPendingPing()`.
We should also cover the code path through `_scanPendingPings()`.

Comment 3

[Kate Ustiuzhanina]

Attachment: discardedPending.patch

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8892057 [details] [diff] [review]
discardedPending.patch

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

::: toolkit/components/telemetry/TelemetryStorage.jsm
@@ +1367,5 @@
>                 .add(Math.floor(fileSize / 1024 / 1024));
>        Telemetry.getHistogramById("TELEMETRY_PING_SIZE_EXCEEDED_PENDING").add();
>        TelemetryStopwatch.cancel("TELEMETRY_PENDING_LOAD_MS");
> +
> +      // String param must be changed to ping.type after resolving Bug 1384903

// Currently we don't have the ping type available without loading the ping from disk. Bug 1384903 will fix that.

@@ +1603,5 @@
>              Telemetry.getHistogramById("TELEMETRY_DISCARDED_PENDING_PINGS_SIZE_MB")
>                       .add(Math.floor(info.size / 1024 / 1024));
>              Telemetry.getHistogramById("TELEMETRY_PING_SIZE_EXCEEDED_PENDING").add();
> +
> +            // String param must be changed to ping.type after resolving Bug 1384903

// Currently we don't have the ping type available without loading the ping from disk. Bug 1384903 will fix that.

::: toolkit/components/telemetry/docs/data/health-ping.rst
@@ +65,4 @@
>  The ``pingDiscardedForSize`` field contains the information about top ten pings, whose size exceeded the
>  ping size limit (1 mb). This field lists the number of discarded pings per ping type.
>  
> +The ping type "<unknown>" is used for indication that pending ping's size exceeded the limit.

"... used to indicate that a pending ..."
"This is because we don't have the pending pings type available cheaply at the moment."

Comment 5

[Kate Ustiuzhanina]

Attachment: discardedPending.patch

Comment 6

[Kate Ustiuzhanina]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c642439e2a94cd23c22846aee1be0c4c65145db

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67456afc416
Track dicarded for size pending ping with TelemetryHealthPing. r=gfritzsche

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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