Telemetry doesn't save pending pings on shutdown

RESOLVED FIXED in Firefox 29

Status

()

Toolkit
Telemetry
P1
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rvitillo, Assigned: rvitillo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
x86
All
Points:
---

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Telemetry submissions are decreasing after fixing Bug 839794. It seems that pending pings are not saved properly on shutdown ("profile-before-change2") by TelemetryFile.savePingToFile due to the following error: "OS.File has been shut down." The error is not triggered by the xpcshell tests though.
(Assignee)

Comment 1

3 years ago
Created attachment 8369669 [details] [diff] [review]
Bug 967203 - Telemetry doesn't save pending pings on shutdown, v1
Assignee: nobody → rvitillo
Status: NEW → ASSIGNED
Attachment #8369669 - Flags: review?(dteller)
Comment on attachment 8369669 [details] [diff] [review]
Bug 967203 - Telemetry doesn't save pending pings on shutdown, v1

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +894,5 @@
>        Telemetry.canRecord = false;
>        return;
>      }
> +
> +    AsyncShutdown.profileBeforeChange.addBlocker(

Telemetry uses profile-before-change2 specifically because many components use profile-before-change to collect their data.

You should rather add a phase sendTelemetry to AsyncShutdown, backed by notification profile-before-change2, and use this phase here.

@@ +901,5 @@
> +        this.uninstall();
> +        if (Telemetry.canSend) {
> +          return this.savePendingPings();
> +        } else {
> +          return Promise.resolve();

If we don't have to wait for anything, just don't return anything.
Attachment #8369669 - Flags: review?(dteller) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8370089 [details] [diff] [review]
Telemetry doesn't save pending pings on shutdown, v2
Attachment #8369669 - Attachment is obsolete: true
Attachment #8370089 - Flags: review?(dteller)
Comment on attachment 8370089 [details] [diff] [review]
Telemetry doesn't save pending pings on shutdown, v2

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

Looks good.
Attachment #8370089 - Flags: review?(dteller) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/77095ca6e53b
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/77095ca6e53b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
Target Milestone: mozilla29 → mozilla30
(Assignee)

Comment 7

3 years ago
Comment on attachment 8370089 [details] [diff] [review]
Telemetry doesn't save pending pings on shutdown, v2

[Approval Request Comment]
The patch has landed a week ago and is needed to restore Telemetry's data collection.
Attachment #8370089 - Flags: approval-mozilla-aurora?
Attachment #8370089 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab0a84051348
status-firefox29: --- → fixed
status-firefox30: --- → fixed

Updated

3 years ago
Keywords: verifyme
Can this fix be tested manually? If so can you offer some steps in order to check that this issue is fixed?
Flags: needinfo?(rvitillo)
Telemetry submissions recovered on the server side so the issue has been fixed (see http://ec2-50-112-66-71.us-west-2.compute.amazonaws.com:4352/#sandboxes/TelemetryChannelMetrics60DaysAggregator/outputs/TelemetryChannelMetrics60DaysAggregator.nightly.cbuf)
Flags: needinfo?(rvitillo)

Updated

3 years ago
Keywords: verifyme
Blocks: 916078
You need to log in before you can comment on or make changes to this bug.