Closed Bug 1336360 Opened 7 years ago Closed 7 years ago

Use the PingSender for sending pings when the browser shuts down

Categories

(Toolkit :: Telemetry, defect, P1)

Product:
Toolkit
Component:
Telemetry
Type:
defect
Points:
2

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.
59 bytes, text/x-review-board-request
chutten
: review+
Details 
In bug 1310703 the PingSender executable was created and packaged for Firefox Desktop. This new component provides opportunistic ping sending out of the Firefox process.

We should change Telemetry so that "shutdown" (and probably "saved-session"?) pings are handed over to the PingSender when Firefox shuts down.
Points: --- → 2
Depends on: 1310703
Priority: -- → P2
Whiteboard: [measurement:client]
We want this to be one train behind the one bug 1310703 lands on, enabled/disabled by a boolean pref like "toolkit.telemetry.pingsender".

One simply strategy here could be to just hand the ping to the ping sender at [1], when we serialize them on the disk.

Open questions:

- The PingSender can silently fail, so we should still have a copy of the shutdown ping among the pending-pings that are sent on the next session. Will deduplication be handled on the server side?

[1] - http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/toolkit/components/telemetry/TelemetrySession.jsm#1739
(In reply to Alessio Placitelli [:Dexter] from comment #1)
> - The PingSender can silently fail, so we should still have a copy of the
> shutdown ping among the pending-pings that are sent on the next session.
> Will deduplication be handled on the server side?

Only if the ping id is the same as I've discovered.
A cleaner approach (and probably better compared to the one in comment 1) would be similar to the one that Gabriele implemented in bug 1310703.

Basically, we should be adding an option to "submitExternalPing" to let pings be sent with the PingSender as well.
We can add an option, but currently we just want to cover the special case of main pings on shutdown.
Let's keep it limited to that for now.

I previously suggested to handle shutdown pings like this:
- let Firefox Telemetry save them to disk as pending
- pass the full path (or the id?) to the sender
- sender tries to send
- if successful, delete ping
- when Firefox starts up and the ping still exists, it will send it as usual
That way we have a safe fallback, a simple implementation and should have a small amount of duplicates.
Priority: P2 → P1
Blocks: 1340535
Assignee: nobody → alessio.placitelli
Attachment #8841520 - Flags: review?(gsvelto)
@Gabriele, can you take a look at the changes in the PingSender code?
@Georg, does the approach look ok to you? This mimicks what was written in comment 5. Moreover, I found (well, I knew about that but I forgot :-D) that persisted pending pings are uncompressed, so there was no need to bring in the LZ4 dependency.
Status: NEW → ASSIGNED
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117162

High-level i wonder if we need to maintain two paths for pings.
Could we just use the same file-based approach for both crash and shutdown pings?

::: toolkit/components/telemetry/Telemetry.cpp:2906
(Diff revision 2)
>  
>    return NS_ERROR_NOT_IMPLEMENTED;
>  #else // Windows, Mac, Linux, etc...
>    // Obtain the path of the pingsender executable
>    nsAutoString path;
>    nsresult rv = LocatePingSender(path);

Is pass-by-reference or pass-by-pointer our coding standard here?

::: toolkit/components/telemetry/Telemetry.cpp:2915
(Diff revision 2)
> +  // If the optional ping id was provided, pass it to the PingSender
> +  // instead of creating a pipe to send the ping content.

Do we need to add a separate code path here and to the ping sender?
Or can we just make the crash reporter use the same approach?

::: toolkit/components/telemetry/Telemetry.cpp:2917
(Diff revision 2)
> +  UniquePtr<char[]> pingSenderPath(ToNewCString(path));
> +  UniquePtr<char[]> serverURL(ToNewCString(aUrl));
> +
> +  // If the optional ping id was provided, pass it to the PingSender
> +  // instead of creating a pipe to send the ping content.
> +  if (optional_argc) {

Nit: `... == 1`?

::: toolkit/components/telemetry/Telemetry.cpp:2918
(Diff revision 2)
> +    // Get the profile directory and pass it to the ping sender so that it
> +    // knows where to find the pings.

Fix: This gets the full ping path, not just the directory.

::: toolkit/components/telemetry/TelemetrySend.jsm:209
(Diff revision 2)
>     *
>     * @param {Object} ping The ping data to send, must be serializable to JSON.
> +   * @param {Boolean} [usePingSender=false] if true, send the ping using the PingSender.
>     * @return {Promise} Test-only - a promise that is resolved when the ping is sent or saved.
>     */
> -  submitPing(ping) {
> +  submitPing(ping, usePingSender=false) {

This (and below) would probably be more readable with an options object argument?
`...submitPing(p, {usePingSender:true})`
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117162

I think that would be ideal, I can't think of any issue doing that.

@Gabriele, what's your take on this?
Flags: needinfo?(gsvelto)
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117322

::: toolkit/components/telemetry/Telemetry.cpp:2906
(Diff revision 2)
>  
>    return NS_ERROR_NOT_IMPLEMENTED;
>  #else // Windows, Mac, Linux, etc...
>    // Obtain the path of the pingsender executable
>    nsAutoString path;
>    nsresult rv = LocatePingSender(path);

In [this](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#COM_and_pointers) section it seems to suggest using pointers. However, [using references](http://searchfox.org/mozilla-central/search?q=%2C+nsAString%26&case=false&regexp=false&path=) seems to be more common in the codebase compared to using [pointers](http://searchfox.org/mozilla-central/search?q=%2C+nsAString*&case=false&regexp=false&path=).
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117162

> Do we need to add a separate code path here and to the ping sender?
> Or can we just make the crash reporter use the same approach?

The change itself to do that is trivial, as we'd just need to change [here](http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/crashreporter/client/ping.cpp#311) to save *ping* to *ProfileDir/saved-telemetry-pings/<UUID>*. The problem is that I'm not sure how to get the profile directory from the crashreported, assuming that's possible at all.
Ted, bugging you as Gabriele is away... sorry! :-)

Is there any way to get the path to the user profile from the Crash Reporter?
Do we actually know the path to the profile if we actually crash before the profile init?

Thanks!
Flags: needinfo?(ted)
Blocks: 1343277
As we discussed on IRC let's keep both ways of passing the pings for the time being; the pingsender will be rewritten in Rust soon enough so we can delay the choice until then.

In general accessing the profile directory from the crashreporter client is inconvenient since we pass it via environment variables (see https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/toolkit/crashreporter/nsExceptionHandler.cpp#2765); besides it would open up synchronization problems if Firefox is restarted before the crashreporter client shuts down so let's not go there now.
Flags: needinfo?(ted)
Flags: needinfo?(gsvelto)
(In reply to Alessio Placitelli [:Dexter] from comment #14)
> Is there any way to get the path to the user profile from the Crash Reporter?
> Do we actually know the path to the profile if we actually crash before the
> profile init?

Just to clarify: no, the crashreporter client hasn't historically known about the profile directory, since it's very possible to crash before a profile was selected.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117162

> The change itself to do that is trivial, as we'd just need to change [here](http://searchfox.org/mozilla-central/rev/4039fb4c5833706f6880763de216974e00ba096c/toolkit/crashreporter/client/ping.cpp#311) to save *ping* to *ProfileDir/saved-telemetry-pings/<UUID>*. The problem is that I'm not sure how to get the profile directory from the crashreported, assuming that's possible at all.

After discussing this over IRC, I've filed bug 1343504. Let's keep both paths here and take further discussions to the other bug.
Attachment #8841520 - Flags: review?(gsvelto)
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117820

As discussed on IRC I think it's better to always pass the ping content via the pipe as well as always passing the ping path. This way the ping sender will not need to read the ping from disk, but can still delete the ping file if it successfully sent it. This way we wouldn't need two separate paths in the ping sender and `Telemetry.runPingSender()` wouldn't need to have an optional argument either.

::: toolkit/components/telemetry/Telemetry.cpp:2894
(Diff revision 3)
> +
>  #endif // MOZ_WIDGET_ANDROID
>  
>  NS_IMETHODIMP
> -TelemetryImpl::RunPingSender(const nsACString& aUrl, const nsACString& aPing)
> +TelemetryImpl::RunPingSender(const nsACString& aUrl, const nsACString& aPing,
> +                             const nsACString& aPingId, uint8_t optional_argc)

Rather than having the arguments, make the ping id mandatory. The only consumer of this function will use it so there's no point in making it optional.

::: toolkit/components/telemetry/Telemetry.cpp:2917
(Diff revision 3)
> +  UniquePtr<char[]> pingSenderPath(ToNewCString(path));
> +  UniquePtr<char[]> serverURL(ToNewCString(aUrl));
> +
> +  // If the optional ping id was provided, pass it to the PingSender
> +  // instead of creating a pipe to send the ping content.
> +  if (optional_argc == 1) {

If you make the ping ID mandatory and you pass the ping via the pipe you can simplify all the code below since you don't need two separate paths anymore.

::: toolkit/components/telemetry/TelemetrySend.jsm:716
(Diff revision 3)
> +   * submissions.
> +   *
> +   * @param {String} pingId The id of the ping to send.
> +   * @param {String} submissionURL The complete Telemetry-compliant URL for the ping.
> +   */
> +  _sendWithPingSender(pingId, submissionURL) {

If you pass the ping itself instead of the ping ID later you can...

::: toolkit/components/telemetry/TelemetrySend.jsm:719
(Diff revision 3)
> +   * @param {String} submissionURL The complete Telemetry-compliant URL for the ping.
> +   */
> +  _sendWithPingSender(pingId, submissionURL) {
> +    this._log.trace("_sendWithPingSender - sending " + pingId + " to " + submissionURL);
> +    try {
> +      Telemetry.runPingSender(submissionURL, undefined, pingId);

... pass the stringified ping so that it gets sent via the pipe:

`Telemetry.runPingSender(submissionURL, JSON.stringify(ping), ping.id)`

::: toolkit/components/telemetry/nsITelemetry.idl:555
(Diff revision 3)
> +   *        the ping is deleted from the disk.
>     *
>     * @throws NS_ERROR_FAILURE if we couldn't run the pingsender or if we
>     *         couldn't hand it the ping data
>     */
> -  void runPingSender(in ACString aUrl, in ACString aPing);
> +  [optional_argc]

Making `aPingId` mandatory means you don't need the optional argument here.

::: toolkit/components/telemetry/pingsender/pingsender.cpp:43
(Diff revision 3)
>    }
>  
>    return ping;
>  }
>  
> +/**

You can remove this if you pass the ping contents via the pipe, leave only the part where you delete the ping if the ping ID was passed as an argument.

::: toolkit/components/telemetry/pingsender/pingsender.cpp:82
(Diff revision 3)
> +    pingPath = argv[2];
> +  } else if (argc == 2) {
> +    // Only the URL was provided.
>      url = argv[1];
>    } else {
> -    PINGSENDER_LOG("Usage: pingsender URL\n"
> +    PINGSENDER_LOG("Usage: pingsender URL [<ping path>]\n"

Adjust the comment to reflect the fact that the ping ID is optional but the ping payload is always passed via `stdin`.

::: toolkit/components/telemetry/pingsender/pingsender.cpp:98
(Diff revision 3)
>    }
>  
> -  string ping(ReadPingFromStdin());
> +  // If the path to a ping was provided, read the file and feed it into "ping".
> +  // Otherwise read from stdin.
> +  bool usingPingFile = !pingPath.empty();
> +  string ping(usingPingFile ? ReadPingFromFile(pingPath) : ReadPingFromStdin());

No need for this either.

::: toolkit/components/telemetry/pingsender/pingsender.cpp:109
(Diff revision 3)
>  
>    if (!Post(url, ping)) {
>      exit(EXIT_FAILURE);
>    }
>  
> +  if (usingPingFile && remove(pingPath.c_str())) {

When I wrote the code for the `Post()` method I didn't take into account that we might be interested in the HTTP response so I'm not sure if the Windows implementation will return false if it gets a 4xx response. If you want to ensure that the ping was successfully sent at this point you'll need to verify that. The implementation in  `pingsender_unix_common.cpp` does return false if the sender responds with 4xx so that should already work reliably.

::: toolkit/components/telemetry/tests/unit/test_PingSender.js:83
(Diff revision 3)
> +               "Should have received the correct ping type.");
> +  Assert.deepEqual(ping.payload, PENDING_PING_DATA.payload,
> +                   "Should have received the correct payload.");
> +
> +  // Check that the PingSender removed the pending ping.
> +  let pendingPingList = yield TelemetryStorage.loadPendingPingList();

Note that this is asynchronous: the pingsender will delete the ping after it has sent it so there's a chance for a race condition here. Unfortunately this cannot be fixed unless you make the code in `Telemetry.runPingSender()` block until the pingsender quits when using it in a test. This is possible though it might be a lot of extra work just to make this test robust (generally speaking we don't want to wait for the pingsender to exit because that'd defeat the point of using the pingsender in the first place).
Attachment #8841520 - Flags: review?(gsvelto) → review-
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review117820

> When I wrote the code for the `Post()` method I didn't take into account that we might be interested in the HTTP response so I'm not sure if the Windows implementation will return false if it gets a 4xx response. If you want to ensure that the ping was successfully sent at this point you'll need to verify that. The implementation in  `pingsender_unix_common.cpp` does return false if the sender responds with 4xx so that should already work reliably.

Good catch. *HttpOpenRequest* doesn't fail on 4xx and 5xx. [HttpQueryInfo](https://msdn.microsoft.com/it-it/library/windows/desktop/aa384238(v=vs.85).aspx) must be used to get the HTTP status code.
Blocks: 1343832
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review118236

LGTM with only a couple of nits to address.

::: toolkit/components/telemetry/pingsender/pingsender_win.cpp:110
(Diff revision 5)
>                              /* lpszHeaders */ nullptr,
>                              /* dwHeadersLength */ 0,
>                              (LPVOID)payload.c_str(),
>                              payload.size());
>    if (!rv) {
>      PINGSENDER_LOG("ERROR: Could not execute HTTP POST request\n");

nit: You want to return false here, since the HTTP POST wasn't even executed

::: toolkit/components/telemetry/pingsender/pingsender_win.cpp:122
(Diff revision 5)
> +                     /* dwInfoLevel */ HTTP_QUERY_STATUS_CODE | HTTP_QUERY_FLAG_NUMBER,
> +                     /* lpvBuffer */ &statusCode,
> +                     /* lpdwBufferLength */ &bufferLength,
> +                     /* lpdwIndex */ NULL);
> +  if (!rv) {
> +    PINGSENDER_LOG("ERROR: Could not get the HTTP status code\n");

nit: Likewise.

::: toolkit/components/telemetry/tests/unit/test_PingSender.js:25
(Diff revision 5)
> -  dir.initWithPath(OS.Constants.Path.libDir);
> -
> -  Services.dirsvc.registerProvider({
> -    getFile(aProp, aPersistent) {
> -      aPersistent.value = true;
> -      if (aProp == XRE_APP_DISTRIBUTION_DIR) {
> + * Wait for a ping file to be deleted from the pending pings directory.
> + */
> +function waitForPingDeletion(pingId) {
> +  const path = OS.Path.join(TelemetryStorage.pingDirectoryPath, pingId);
> +
> +  let checkFn = (resolve, reject) => setTimeout(() => {

I think this should be enough since the race is very small, but let's keep our eyes peeled in case it turns orange.
Attachment #8841520 - Flags: review?(gsvelto) → review+
For informational purposes, "main" pings with reason "shutdown" make up the overwhelming majority of all "main" pings submitted to Telemetry. This change should reduce the delay of receiving about 80%[1] of all "main" pings.

This is huge, important, amazing work. I can't wait for it to land.

[1]: https://sql.telemetry.mozilla.org/queries/3434/source#6702
(In reply to Chris H-C :chutten from comment #24)
> For informational purposes, "main" pings with reason "shutdown" make up the
> overwhelming majority of all "main" pings submitted to Telemetry. This
> change should reduce the delay of receiving about 80%[1] of all "main" pings.
> 
> This is huge, important, amazing work. I can't wait for it to land.
> 
> [1]: https://sql.telemetry.mozilla.org/queries/3434/source#6702

Yay! \o/

There's one thing you just made me realize though: bug 1333128 is all of a sudden a lot more important unless we're prepared with a ten-fold increase in bandwidth when receiving pings.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

Temporarily cancelling the review request due to a problem in the code!
Attachment #8841520 - Flags: review?(gfritzsche)
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review118638

Some comments and questions below without going through all details yet.
Also, i'm missing updates for the docs for:
- pingsender
- prefs

::: toolkit/components/telemetry/TelemetrySend.jsm:737
(Diff revision 5)
>        return Promise.resolve();
>      }
>  
> +    // Send the ping using the PingSender, if requested.
> +    if (options.usePingSender) {
> +      // Build the Telemetry submission URL.

This should be shared logic (helper function).

::: toolkit/components/telemetry/nsITelemetry.idl:548
(Diff revision 5)
> +   * @param aPingId A string UUID. If provided, the PingSender will delete
> +   *        ping from the disk if sent successfully.

Why the uuid instead of the path?
The JS Telemetry modules already has the pending path logic, why do we need to duplicate it in C++?

::: toolkit/components/telemetry/tests/unit/head.js:186
(Diff revision 5)
>    AddonTestUtils.registerDirectory("XREAppFeat", distroDir);
>    return AddonTestUtils.promiseStartupManager();
>  }
>  
> +function setupPingSender() {
> +  // Make sure the code can find the pingsender executable

Why is this needed?

::: toolkit/components/telemetry/tests/unit/test_PingSender.js:16
(Diff revision 5)
> -add_task(function* test_pingSender() {
> -  // Make sure the code can find the pingsender executable
> +const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
> +const PREF_TELEMETRY_SERVER = "toolkit.telemetry.server";

Should we share those in head.js?
Here... or in mentored follow-up bug that addresses all the shared pref vars.

::: toolkit/components/telemetry/tests/unit/test_PingSender.js:22
(Diff revision 5)
> -  // Make sure the code can find the pingsender executable
> -  const XRE_APP_DISTRIBUTION_DIR = "XREAppDist";
> -  let dir = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> -  dir.initWithPath(OS.Constants.Path.libDir);
> -
> -  Services.dirsvc.registerProvider({
> +const PREF_TELEMETRY_SERVER = "toolkit.telemetry.server";
> +
> +/**
> + * Wait for a ping file to be deleted from the pending pings directory.
> + */
> +function waitForPingDeletion(pingId) {

Instead of this function, let's use one of the existing `waitForCondition()` implementations, making it shared properly if needed.

::: toolkit/components/telemetry/tests/unit/test_PingSender.js:52
(Diff revision 5)
> +  // Allow the test to find the pingsender executable.
> +  setupPingSender();
> +
> +  // Start the ping server and let Telemetry know about it.
>    PingServer.start();
> +  Preferences.set(PREF_TELEMETRY_SERVER, "http://localhost:" + PingServer.port);

What do you need to set this for?

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:1307
(Diff revision 5)
> +  // Make sure the pending pings directory exists as other tests might have
> +  // removed it.

That sounds like we should fix this case in `TelemetryStorage`

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:1318
(Diff revision 5)
> +  // Shutdown telemetry and wait for an incoming ping.
> +  let nextPing = PingServer.promiseNextPing();
> +  yield TelemetryController.testShutdown();
> +  const ping = yield nextPing;
> +
> +  // Check that we received a shutdown ping.

Should we have a (temporary?) data point (outside of the ping data) to tell if a ping was sent with the pingsender?
That might be relevant for data validation and analysis.
If yes, we should have test coverage for it too. mreid should have an opinion on where to put this.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:1323
(Diff revision 5)
> +  // Try again, this time disable ping upload. The PingSender
> +  // should not be sending any ping!

What about the scenarios of:
(1)
- pingsender can't reach server
- telemetry picks up and submits at the next start

(2)
- pingsender gets error code that means resubmit
- telemetry picks up and submits at next restart


(3)
- pingsender gets error code that means delete
- ...?

Do we have proper coverage for these?
For (3), should we handle this in pingsender or leave retry and deletion to fx telemetry (safer)?
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> For (3), should we handle this in pingsender or leave retry and deletion to
> fx telemetry (safer)?

I'd leave complex logic to the telemetry code and keep the ping sender as simple as possible. The idea is to use it in the 90% of cases where it's sufficient, and handle all the other trickier cases outside of its scope.
(In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> ::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js:1318
> (Diff revision 5)
> > +  // Shutdown telemetry and wait for an incoming ping.
> > +  let nextPing = PingServer.promiseNextPing();
> > +  yield TelemetryController.testShutdown();
> > +  const ping = yield nextPing;
> > +
> > +  // Check that we received a shutdown ping.
> 
> Should we have a (temporary?) data point (outside of the ping data) to tell
> if a ping was sent with the pingsender?
> That might be relevant for data validation and analysis.
> If yes, we should have test coverage for it too. mreid should have an
> opinion on where to put this.

Yes, having this data point could be helpful.
Mark, do you have any opinion on this?
Flags: needinfo?(mreid)
(In reply to Alessio Placitelli [:Dexter] from comment #30)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> > Should we have a (temporary?) data point (outside of the ping data) to tell
> > if a ping was sent with the pingsender?
> > That might be relevant for data validation and analysis.
> > If yes, we should have test coverage for it too. mreid should have an
> > opinion on where to put this.
> 
> Yes, having this data point could be helpful.
> Mark, do you have any opinion on this?

Agreed, it would be nice to be able to tell whether a ping was sent by pingsender. What sort of analysis did you have in mind? We could add an HTTP header to identify submissions from pingsender - it's quite easy to add a header value to the records in the ingestion pipeline.
Flags: needinfo?(mreid)
(In reply to Mark Reid [:mreid] from comment #31)
> (In reply to Alessio Placitelli [:Dexter] from comment #30)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #28)
> > > Should we have a (temporary?) data point (outside of the ping data) to tell
> > > if a ping was sent with the pingsender?
> > > That might be relevant for data validation and analysis.
> > > If yes, we should have test coverage for it too. mreid should have an
> > > opinion on where to put this.
> > 
> > Yes, having this data point could be helpful.
> > Mark, do you have any opinion on this?
> 
> Agreed, it would be nice to be able to tell whether a ping was sent by
> pingsender. What sort of analysis did you have in mind? We could add an HTTP
> header to identify submissions from pingsender - it's quite easy to add a
> header value to the records in the ingestion pipeline.

Adding a header is a very good idea! Having this temporary header would be useful for checking:

- How many duplicate pings come from the PingSender (if any);
- How many shutdown pings are successfully sent through the pingsender (that's indirectly measurable by observing the latency too, but having a clear temporary indicator for this is a good idea too);
- How many pings coming from the pingsender are corrupted (if any, hopefully none).
- Clearly measuring the latency of PingSender shutdown pings vs shutdown pings sent through the normal path.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review118638

> Why is this needed?

This was needed in the previous version of the patch due to a bug in |LocatePingSender|. With that fixed, this is not needed anymore.

> That sounds like we should fix this case in `TelemetryStorage`

Whoops, we don't really need these lines. TelemetryStorage already takes care of [that](http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/toolkit/components/telemetry/TelemetryStorage.jsm#1280,1795).

> Should we have a (temporary?) data point (outside of the ping data) to tell if a ping was sent with the pingsender?
> That might be relevant for data validation and analysis.
> If yes, we should have test coverage for it too. mreid should have an opinion on where to put this.

It turns out we're already sending "User-Agent: pingsender/1.0", which should be enough to recognize these pings. I've added test coverage for that.

> What about the scenarios of:
> (1)
> - pingsender can't reach server
> - telemetry picks up and submits at the next start
> 
> (2)
> - pingsender gets error code that means resubmit
> - telemetry picks up and submits at next restart
> 
> 
> (3)
> - pingsender gets error code that means delete
> - ...?
> 
> Do we have proper coverage for these?
> For (3), should we handle this in pingsender or leave retry and deletion to fx telemetry (safer)?

As Gabriele suggested, I'd rather have (3) dealt by Firefox.

As for (1) and (2), anything other than a 200 HTTP status results in leaving the pending ping there.
However, we have no control on the PingSender process when it gets spawned through *runPingSender*, so we don't really know when it terminates, making testing (1) and (2) tricky.

Any suggestion on how to handle that part?
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review118638

> Should we share those in head.js?
> Here... or in mentored follow-up bug that addresses all the shared pref vars.

Filed bug 1344295.
It might still be a good idea to add an "X-" prefixed header for just this new piece of data. We're not currently storing the user agent string, and IIRC it can be quite long and could require storing a fair chunk of extra data (that's mostly repeated in the data contents already). Adding a "X-PingSender-Version: 1.0" header or similar could express the same info more succinctly. What do you think?
(In reply to Mark Reid [:mreid] from comment #36)
> It might still be a good idea to add an "X-" prefixed header for just this
> new piece of data. We're not currently storing the user agent string, and
> IIRC it can be quite long and could require storing a fair chunk of extra
> data (that's mostly repeated in the data contents already). Adding a
> "X-PingSender-Version: 1.0" header or similar could express the same info
> more succinctly. What do you think?

I see, it makes sense to add the new header then.
Gabriele, I've noticed something interesting when trying the patch manually on Windows.

If I run and then shut down Firefox, the ping sender indeed gets called (nothing odd here). However, a black command line prompt shows up for just a second. Are you aware of any way to suppress that?
Flags: needinfo?(gsvelto)
We can either use the right flag to CreateProcess to suppress the console window, or we can have pingsender built as a GUI app, not a console app.
The pingsender is launched from Telemetry using PR_CreateProcessDetached(). The only way to force it to use the CREATE_NO_WINDOW flag is by pretending we're redirecting stdout:

https://dxr.mozilla.org/mozilla-central/rev/8d026c60151005ad942e3d4389318fe28a0c8c54/nsprpub/pr/src/md/windows/ntmisc.c#650

Try adding this line:

PR_ProcessAttrSetStdioRedirect(attr, PR_StandardOutput, PR_GetSpecialFD(PR_StandardOutput));

After the line where we set the redirect for PR_StandardInput in TelemetryImpl::RunPingSender()

I hadn't thought about it when I wrote this code because in the other cases we were calling CreateProcess() directly with the appropriate flag.
Flags: needinfo?(gsvelto)
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review118638

> Instead of this function, let's use one of the existing `waitForCondition()` implementations, making it shared properly if needed.

Using a "provided" function, as the one found [here](https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/browser/head.js#1428), make things a bit trickier: OS.File().exists() is async, so we have to wait for it to complete before having a result.

> As Gabriele suggested, I'd rather have (3) dealt by Firefox.
> 
> As for (1) and (2), anything other than a 200 HTTP status results in leaving the pending ping there.
> However, we have no control on the PingSender process when it gets spawned through *runPingSender*, so we don't really know when it terminates, making testing (1) and (2) tricky.
> 
> Any suggestion on how to handle that part?

I've added a test that covers the case when we receive an error from the server (e.g. 404), as discussed.
The code paths in the pingsender for not reaching the server or receiving an error are the same: both return early leaving the ping in place.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

Gabriele, I'm requesting a review again as some things have changed in both the PingSender and Telemetry.cpp:

- The PingSender is now sending a new header together with the pings, to help with data validation.
- Telemetry.cpp is looking for the pingsender executable in a different path. The old didn't work when running Firefox outside of the test. Also, using the new one, allows us to get rid of the setup code needed for xpcshell-tests.
- We're redirecting the stdout when launching the pingsender using Telemetry to prevent the command window from spawning when Firefox shuts down.
Attachment #8841520 - Flags: review+ → review?(gsvelto)
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review119594

Still looking good with a few nits.

::: toolkit/components/telemetry/Telemetry.cpp:2917
(Diff revisions 7 - 8)
>      return NS_ERROR_FAILURE;
>    }
>  
>    // Connect the pingsender standard input to the pipe and launch it
>    PR_ProcessAttrSetStdioRedirect(attr, PR_StandardInput, pipeRead);
> +  // Make sure we don't display a command prompt on Windows.

nit: Since this is a trick let's make it more explicit, something like "We pretend we're redirecting stdout to force NSPR not to show a command prompt when launching the program."

::: toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp:120
(Diff revision 8)
>  
>    if (!easy_init ||
>        !easy_setopt ||
>        !easy_perform ||
>        !easy_getinfo ||
> +      !slist_append ||

nit: Check for `slist_free_all` too for completeness

::: toolkit/components/telemetry/pingsender/pingsender_unix_common.cpp:144
(Diff revision 8)
>  {
>    easy_setopt(mCurl, CURLOPT_URL, url.c_str());
>    easy_setopt(mCurl, CURLOPT_USERAGENT, kUserAgent);
>  
> +  // Set the custom version header.
> +  curl_slist *headerChunk = nullptr;

nit: `curl_slist* headerChunk = nullptr;`
Attachment #8841520 - Flags: review?(gsvelto) → review+
Is this going to stop telemetry writing session length data etc. to prefs right before shutdown? That would be really nice for shutdown responsiveness, see my last comment on bug 789945. :-)
(In reply to :Gijs from comment #48)
> Is this going to stop telemetry writing session length data etc. to prefs
> right before shutdown? That would be really nice for shutdown
> responsiveness, see my last comment on bug 789945. :-)

Unfortunately no. The shutdown flow will be identical, we will just try to send the shutdown ping with another process.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

As discussed, I'm cancelling the review request here since this is blocked on bug 1343504.
Attachment #8841520 - Flags: review?(gfritzsche)
Depends on: 1343504
Blocks: 1350044
Depends on: 1345153
Chris, this is rebased on bug 1345153: I've removed all the changes to the pingsender from this patch since they're landing with the other one. The patch now only contains changes to the Telemetry part.

Even if 1345153 should be fairly stable, even if it didn't land yet: we reviewed the Telemetry part and it's just pending the crashmanager bits.
Comment on attachment 8841520 [details]
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down.

https://reviewboard.mozilla.org/r/115722/#review127222

Seems straightforward to me!

::: toolkit/components/telemetry/TelemetrySend.jsm:793
(Diff revision 11)
> +      this._log.error("_sendWithPingSender - failed to submit shutdown ping", e);
> +    }
> +  },
> +
> +  submitPing(ping, options) {
> +    this._log.trace("submitPing - ping id: " + ping.id + ", options: " + JSON.stringify(options));

Here's hoping options isn't self-referential :)

::: toolkit/components/telemetry/docs/internals/preferences.rst:55
(Diff revision 11)
>  
>    Sets whether to dump Telemetry log messages to ``stdout`` too.
>  
> +``toolkit.telemetry.shutdownPingSender.enabled``
> +
> +  Allow the ``shutdown`` ping to be sent when the browser shuts down, instead of the next restart, using the ping sender.

Should we link to the pingsender doc here?
Attachment #8841520 - Flags: review?(chutten) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e5ede3cb7c6a
Use the PingSender for sending pings when the browser shuts down. r=chutten,gsvelto
https://hg.mozilla.org/mozilla-central/rev/e5ede3cb7c6a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Abe, do you have cycles to carry on the QA work on this bug (since you already have context about the pingsender and put together the plan on bug 1340535)?
Flags: needinfo?(amasresha)
Attachment #8842800 - Attachment is obsolete: true
I was expecting it. Thanks for the ni.
I do not see Linux opt build in comment 58. Shall we wait for the normal nightly to come out, that will be on Monday?  
But we can start testing on Windows and Mac now.
Flags: needinfo?(amasresha)
Flags: needinfo?(alessio.placitelli)
(In reply to Abe - QA (:Abe_LV) from comment #60)
> I was expecting it. Thanks for the ni.
> I do not see Linux opt build in comment 58. Shall we wait for the normal
> nightly to come out, that will be on Monday?  
> But we can start testing on Windows and Mac now.

Monday is fine, let's start the new week with some proper QA test coverage! :-D
Flags: needinfo?(alessio.placitelli)
Depends on: 1354231
Depends on: 1354482
Depends on: 1364145
Mark 54 won't fix as this is late for Beta54.
status-firefox54: affected → wontfix
It seems that some users have noticed this problem, after using Firefox and shut down will appear a command window that opens briefly and then immediately closes. This seems puzzling, the user can not determine whether it is part of the Firefox program, as some users commented that he was worried about whether it was an implanted Trojan. Maybe we need to improve the way to submit telemetry data.

Some related discussion：
http://mozilla.com.cn/thread-390692-1-1.html (chinese)
https://www.reddit.com/r/firefox/comments/68c59k/nightly_pingsenderexe/
http://forums.mozillazine.org/viewtopic.php?f=7&t=3032530
https://support.mozilla.org/zh-CN/questions/1158361
(In reply to yxu from comment #64)
> It seems that some users have noticed this problem, after using Firefox and
> shut down will appear a command window that opens briefly and then
> immediately closes. This seems puzzling, the user can not determine whether
> it is part of the Firefox program, as some users commented that he was
> worried about whether it was an implanted Trojan. Maybe we need to improve
> the way to submit telemetry data.
> 
> Some related discussion：
> http://mozilla.com.cn/thread-390692-1-1.html (chinese)
> https://www.reddit.com/r/firefox/comments/68c59k/nightly_pingsenderexe/
> http://forums.mozillazine.org/viewtopic.php?f=7&t=3032530
> https://support.mozilla.org/zh-CN/questions/1158361

This problem was fixed on Nightly (after being reported) by bug 1360819.
All these comments seem to be prior to that fix.
(In reply to Alessio Placitelli [:Dexter] from comment #65)
> This problem was fixed on Nightly (after being reported) by bug 1360819.
> All these comments seem to be prior to that fix.

Actually not all of those comments are about the window popping up, some describe pingsender as running even though the user has telemetry and FHR enabled and they're from August. I think those are worth some investigation.
Depends on: 1399433
(In reply to Gabriele Svelto [:gsvelto] from comment #66)
> (In reply to Alessio Placitelli [:Dexter] from comment #65)
> > This problem was fixed on Nightly (after being reported) by bug 1360819.
> > All these comments seem to be prior to that fix.
> 
> Actually not all of those comments are about the window popping up, some
> describe pingsender as running even though the user has telemetry and FHR
> enabled and they're from August. I think those are worth some investigation.

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

Attachment

General

Creator:
Created:
Updated:
Size: