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

RESOLVED FIXED in Firefox 57

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: katejimmelon, Assigned: katejimmelon)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Priority: -- → P3
(Assignee)

Updated

2 years ago
Priority: P3 → P1
(Assignee)

Updated

2 years ago
Assignee: nobody → kustiuzhanina
(Assignee)

Comment 1

2 years ago
Created attachment 8890894 [details] [diff] [review]
discardedPending.patch
Attachment #8890894 - Flags: review?(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()`.
Attachment #8890894 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 3

2 years ago
Created attachment 8892057 [details] [diff] [review]
discardedPending.patch
Attachment #8890894 - Attachment is obsolete: true
Attachment #8892057 - Flags: review?(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."
Attachment #8892057 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8894465 [details] [diff] [review]
discardedPending.patch
Attachment #8892057 - Attachment is obsolete: true
Attachment #8894465 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 7

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b67456afc416
Track dicarded for size pending ping with TelemetryHealthPing. r=gfritzsche
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b67456afc416
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.