Closed Bug 1244859 Opened 4 years ago Closed 4 years ago

Remove trailing slash from telemetry ping url

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox45 --- fixed
firefox46 --- fixed
firefox47 --- fixed
fennec 45+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

13:12 <@mreid> mcomella: you should remove that trailing slash though
13:12 <@mreid> (it doesn't appear to affect anything on the server, it's just not to-spec)
13:13 <mcomella> mreid: So `.../buildId` as opposed to `.../buildId/` ?
13:13 <@mreid> mcomella: yep
It was not to-spec (though it doesn't appear to have an effect on how the
server reads the data).

Review commit: https://reviewboard.mozilla.org/r/33097/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33097/
Attachment #8714470 - Flags: review?(mark.finkle)
Only bothering to get a review so we can uplift later.
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle

https://reviewboard.mozilla.org/r/33097/#review29931
Attachment #8714470 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/3e53d5cf6400
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Let's make sure this gets uplifted with bug 1205835.
Blocks: 1205835
No longer depends on: 1205835
tracking-fennec: --- → 45+
(It's not strictly necessary for bug 1205835 though, just an optimization)
No longer blocks: 1205835
Depends on: 1205835
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle

This is a micro-optimization on the implementation in bug 1205835, which must be uplifted first.

Approval Request Comment
[Feature/regressing bug #]: bug 1205835.
[User impact if declined]: Users send an extra byte (the trailing url slash) when requesting a url. Otherwise, no impact. It's not technically correct according to the server spec but it handles it gracefull.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Extremely low – we remove a character from a url string. Worst case, we upload to the wrong url and break uploads.

[String/UUID change made/needed]: None
Attachment #8714470 - Flags: approval-mozilla-beta?
Attachment #8714470 - Flags: approval-mozilla-aurora?
Comment on attachment 8714470 [details]
MozReview Request: Bug 1244859 - Remove trailing slash in telemetry urls. r=mfinkle r=mfinkle

Minor change, taking it.
Should be in 45 beta 4.
Attachment #8714470 - Flags: approval-mozilla-beta?
Attachment #8714470 - Flags: approval-mozilla-beta+
Attachment #8714470 - Flags: approval-mozilla-aurora?
Attachment #8714470 - Flags: approval-mozilla-aurora+
This can't be uplifted without bug 1205835 as it changes files introduced in it. As such, it looks like these uplifted patches created the files in the state they build on top of, when they should be changing one line.

Can this get backed out until bug 1205835 lands?
Flags: needinfo?(sledru)
Flags: needinfo?(cbook)
This (and probably a bunch of the related uplift requests) needs to be rebased to work around the lack of bug 1107811 on the release branches.
Flags: needinfo?(michael.l.comella)
I had to back this out on both branches so that everything else could land cleanly and in the correct order before finally relanding it.

The backouts are in the following commits:
https://hg.mozilla.org/releases/mozilla-aurora/rev/b923a7b09924
https://hg.mozilla.org/releases/mozilla-beta/rev/9b566531611e
Flags: needinfo?(michael.l.comella)
Wes did it.
Michael, do we want to land this again? Thanks
Flags: needinfo?(sledru) → needinfo?(michael.l.comella)
Flags: needinfo?(cbook)
(In reply to Sylvestre Ledru [:sylvestre] from comment #17)
> Wes did it.
> Michael, do we want to land this again? Thanks

This is already landed – the commits are posted out of order. The backout in comment 16 occurred before the relanding in comment 15 & comment 14.
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.