Last Comment Bug 967203 - Telemetry doesn't save pending pings on shutdown
: Telemetry doesn't save pending pings on shutdown
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: x86 All
P1 critical (vote)
: mozilla30
Assigned To: Roberto Agostino Vitillo (:rvitillo)
:
: Georg Fritzsche [:gfritzsche]
Mentors:
Depends on:
Blocks: AsyncShutdown
  Show dependency treegraph
 
Reported: 2014-02-03 13:19 PST by Roberto Agostino Vitillo (:rvitillo)
Modified: 2014-07-06 10:40 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Bug 967203 - Telemetry doesn't save pending pings on shutdown, v1 (4.18 KB, patch)
2014-02-03 14:06 PST, Roberto Agostino Vitillo (:rvitillo)
dteller: feedback+
Details | Diff | Splinter Review
Telemetry doesn't save pending pings on shutdown, v2 (4.81 KB, patch)
2014-02-04 07:39 PST, Roberto Agostino Vitillo (:rvitillo)
dteller: review+
sledru: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Roberto Agostino Vitillo (:rvitillo) 2014-02-03 13:19:31 PST
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.
Comment 1 User image Roberto Agostino Vitillo (:rvitillo) 2014-02-03 14:06:56 PST
Created attachment 8369669 [details] [diff] [review]
Bug 967203 - Telemetry doesn't save pending pings on shutdown, v1
Comment 2 User image David Teller [:Yoric] (hard to reach until July 17th - please use "needinfo") 2014-02-04 06:57:18 PST
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.
Comment 3 User image Roberto Agostino Vitillo (:rvitillo) 2014-02-04 07:39:06 PST
Created attachment 8370089 [details] [diff] [review]
Telemetry doesn't save pending pings on shutdown, v2
Comment 4 User image David Teller [:Yoric] (hard to reach until July 17th - please use "needinfo") 2014-02-04 08:51:07 PST
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.
Comment 5 User image Ryan VanderMeulen [:RyanVM] 2014-02-05 10:36:55 PST
https://hg.mozilla.org/integration/fx-team/rev/77095ca6e53b
Comment 6 User image Carsten Book [:Tomcat] 2014-02-06 04:18:37 PST
https://hg.mozilla.org/mozilla-central/rev/77095ca6e53b
Comment 7 User image Roberto Agostino Vitillo (:rvitillo) 2014-02-14 06:47:44 PST
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.
Comment 8 User image Ryan VanderMeulen [:RyanVM] 2014-02-14 10:23:17 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/ab0a84051348
Comment 9 User image Bogdan Maris, QA [:bogdan_maris] 2014-03-19 06:15:48 PDT
Can this fix be tested manually? If so can you offer some steps in order to check that this issue is fixed?
Comment 10 User image Roberto Agostino Vitillo (:rvitillo) 2014-03-21 04:33:24 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.