bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 57

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Kate Ustiuzhanina, Assigned: Kate Ustiuzhanina)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Priority: -- → P3
(Assignee)

Updated

a year ago
Priority: P3 → P1
(Assignee)

Updated

a year ago
Assignee: nobody → kustiuzhanina
(Assignee)

Comment 1

a year 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

a year 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

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

Updated

11 months ago
Keywords: checkin-needed

Comment 7

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b67456afc416
Status: NEW → RESOLVED
Last Resolved: 11 months 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.