1134272 - Sending stored pings in the old ping format is broken.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

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?

Comment 2

[Vladan Djeric (:vladan)]

(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?

Comment 3

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 5

[Georg Fritzsche [: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.

Comment 6

[Georg Fritzsche [: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)?

Comment 7

[Vladan Djeric (:vladan)]

> 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

Comment 8

[Alessio Placitelli [:Dexter]]

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

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: Allows to send legacy pings and adds the relative test coverage (v2)

Checks for the new format instead of the old one.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 11

[Georg Fritzsche [: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.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 13

[Georg Fritzsche [: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.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: Allows to send legacy pings and adds the relative test coverage (v4)

Thanks for the review. This works also with no payload.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b126d41013ff