Closed
Bug 1134272
Opened 10 years ago
Closed 10 years ago
Sending stored pings in the old ping format is broken.
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: Dexter)
References
Details
(Whiteboard: [ready])
Attachments
(1 file, 3 obsolete files)
8.75 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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•10 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)
Comment 2•10 years ago
|
||
(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•10 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•10 years ago
|
||
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•10 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•10 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)
Comment 7•10 years ago
|
||
> 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•10 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•10 years ago
|
||
Checks for the new format instead of the old one.
Attachment #8566640 -
Attachment is obsolete: true
Reporter | ||
Comment 10•10 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•10 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•10 years ago
|
||
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•10 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•10 years ago
|
||
Thanks for the review. This works also with no payload.
Attachment #8567046 -
Attachment is obsolete: true
Attachment #8567194 -
Flags: review+
Reporter | ||
Updated•10 years ago
|
Whiteboard: [ready]
Reporter | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 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.
Description
•