Sending stored pings in the old ping format is broken.

RESOLVED FIXED in Firefox 39

Status

()

Toolkit
Telemetry
RESOLVED FIXED
3 years ago
3 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)

With the ping format changes etc. applied, we currently can't load and send old pings that may be still stored on disk.
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)
(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

3 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)
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)
(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

3 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

3 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
(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)
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

3 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)
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

3 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+
Whiteboard: [ready]
https://hg.mozilla.org/mozilla-central/rev/b126d41013ff
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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.