Sending stored pings in the old ping format is broken.

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [ready])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
With the ping format changes etc. applied, we currently can't load and send old pings that may be still stored on disk.
(Reporter)

Comment 1

4 years ago
Before we go ahead and do work here - can you confirm that we need to submit them?
Or would we possibly be ok with losing old pings?
Flags: needinfo?(vdjeric)
(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> Before we go ahead and do work here - can you confirm that we need to submit
> them?
> Or would we possibly be ok with losing old pings?

Wouldn't it be straightforward to support loading & sending of old pings?
Flags: needinfo?(vdjeric)
(Reporter)

Comment 3

4 years ago
(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #2)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> > Before we go ahead and do work here - can you confirm that we need to submit
> > them?
> > Or would we possibly be ok with losing old pings?
> 
> Wouldn't it be straightforward to support loading & sending of old pings?

Not hard, yes. I'm just wondering if we need to.
(Assignee)

Comment 4

4 years ago
Created attachment 8566640 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage.

This patch modifies TelemetryPing.jsm and TelemetryFile.jsm to allow sending pings in the old format.
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Attachment #8566640 - Flags: review?(gfritzsche)
(Reporter)

Comment 5

4 years ago
Comment on attachment 8566640 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage.

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +97,5 @@
> +/**
> + * Determine if the ping has the old, legacy, ping format.
> + */
> +function isOldPingFormat(aPing) {
> +  return (!aPing.version && !aPing.application && aPing.payload.info);

* !("field" in aPing) is better
* accessing aPing.payload.info will throw if there is no payload field
* i... think we have to support custom pings that other components like Loop drop into the Telemetry pending-ping directory

@@ +507,5 @@
>  
>    submissionPath: function submissionPath(ping) {
> +    // The new ping format contains an "application" section, the old one doesn't.
> +    let pathComponents;
> +    if (isOldPingFormat(ping)) {

I think it's more reliable to detect the new format (has "id", has "version", version>=4, ...).
That also allows us to fall back to the old logic for custom pings that don't have a slug field if needed.
We have to check if we maybe lost handling for that when we changed submission path construction.
Attachment #8566640 - Flags: review?(gfritzsche)
(Reporter)

Comment 6

4 years ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> ::: toolkit/components/telemetry/TelemetryPing.jsm
> @@ +97,5 @@
> > +/**
> > + * Determine if the ping has the old, legacy, ping format.
> > + */
> > +function isOldPingFormat(aPing) {
> > +  return (!aPing.version && !aPing.application && aPing.payload.info);
> 
> * !("field" in aPing) is better
> * accessing aPing.payload.info will throw if there is no payload field
> * i... think we have to support custom pings that other components like Loop
> drop into the Telemetry pending-ping directory
> 
> @@ +507,5 @@
> >  
> >    submissionPath: function submissionPath(ping) {
> > +    // The new ping format contains an "application" section, the old one doesn't.
> > +    let pathComponents;
> > +    if (isOldPingFormat(ping)) {
> 
> I think it's more reliable to detect the new format (has "id", has
> "version", version>=4, ...).
> That also allows us to fall back to the old logic for custom pings that
> don't have a slug field if needed.
> We have to check if we maybe lost handling for that when we changed
> submission path construction.

Vladan, do you know what formats we have to support for backwards compatibility?
Only the ones with a slug field or also without slug (e.g. custom pings without slug that got dropped into the pending-pings directory)?
Flags: needinfo?(vdjeric)
> Vladan, do you know what formats we have to support for backwards
> compatibility?
> Only the ones with a slug field or also without slug (e.g. custom pings
> without slug that got dropped into the pending-pings directory)?

Yes, we need to support the custom pings (e.g. Loop) as well
Flags: needinfo?(vdjeric)
(Assignee)

Comment 8

4 years ago
Thanks for the review, Goerg!

> That also allows us to fall back to the old logic for custom pings that
> don't have a slug field if needed.
> We have to check if we maybe lost handling for that when we changed
> submission path construction.

Do pings without a slug field even exist? TelemetryFile.jsm [1] relies on on the slug field to behave correctly and Loop knows about the slug field [2].

[1] - https://hg.mozilla.org/mozilla-central/annotate/51458a066fda/toolkit/components/telemetry/TelemetryFile.jsm#l266
[2] - https://hg.mozilla.org/mozilla-central/annotate/51458a066fda/browser/components/loop/MozLoopService.jsm#l767
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 9

4 years ago
Created attachment 8566982 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage (v2)

Checks for the new format instead of the old one.
Attachment #8566640 - Attachment is obsolete: true
(Reporter)

Comment 10

4 years ago
(In reply to Alessio Placitelli [:Dexter] from comment #8)
> Thanks for the review, Goerg!
> 
> > That also allows us to fall back to the old logic for custom pings that
> > don't have a slug field if needed.
> > We have to check if we maybe lost handling for that when we changed
> > submission path construction.
> 
> Do pings without a slug field even exist? TelemetryFile.jsm [1] relies on on
> the slug field to behave correctly and Loop knows about the slug field [2].

I'm not sure. As long as we're not sure let's have a fallback for that case.
Flags: needinfo?(gfritzsche)
(Reporter)

Comment 11

4 years ago
We could e.g. just make sure that we have .slug or .id on it and put a new UUID on it if not.
(Assignee)

Comment 12

4 years ago
Created attachment 8567046 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage (v3)

TelemetryPing.jsm now adds the slug if we have a ping in the old format with no slug.
I've updated the test accordingly.
Attachment #8566982 - Attachment is obsolete: true
Attachment #8567046 - Flags: review?(gfritzsche)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8567046 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage (v3)

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +523,5 @@
> +        ping.slug = generateUUID();
> +      }
> +
> +      // Do we have enough info to build a submission URL?
> +      let info = ping.payload.info;

If we don't have ping.payload, this throws. Let's avoid that. Add test-coverage.
Attachment #8567046 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 14

4 years ago
Created attachment 8567194 [details] [diff] [review]
Allows to send legacy pings and adds the relative test coverage (v4)

Thanks for the review. This works also with no payload.
Attachment #8567046 - Attachment is obsolete: true
Attachment #8567194 - Flags: review+
(Reporter)

Updated

4 years ago
Whiteboard: [ready]
https://hg.mozilla.org/mozilla-central/rev/b126d41013ff
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.