Closed
Bug 1273587
Opened 5 years ago
Closed 5 years ago
Add OS to Sync user agent string
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: dcoates, Assigned: markh)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
The current Sync UA string [1] doesn't include the OS. Maybe it should? It would help make our device metrics [2] better. [1] https://github.com/mozilla/gecko-dev/blob/233f30f2377f3df0f3388721901681f432b813fb/services/sync/modules/rest.js#L41 [2] https://sql.telemetry.mozilla.org/queries/96#150
Assignee | ||
Comment 1•5 years ago
|
||
FTR, the user-agent string is built at https://dxr.mozilla.org/mozilla-central/source/services/sync/modules/rest.js#40. The current string on my machine is "Firefox/49.0a1 FxSync/1.51.0.20160516142357.desktop" while the normal UA string is "Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0" - should we just append the "(Windows NT 6.1; WOW64; rv:49.0)" part to our string, parens and all?
Flags: firefox-backlog+
Priority: -- → P1
Comment 2•5 years ago
|
||
Just a note to be aware, that our metrics pipeline contains special-case logic for parsing these user-agetn strings from sync, which may need to be updated if we add things to them.
Comment 3•5 years ago
|
||
Adding :whd for awareness re: potential changes to this string
Reporter | ||
Comment 4•5 years ago
|
||
If we made the string like this: "Firefox/46.0 (Windows NT 6.1; WOW64; rv:46.0) FxSync/1.51.0.20160516142357.desktop" The current version of lua_sandbox/common_log_format.lua, Just Works™.
Assignee | ||
Comment 5•5 years ago
|
||
The custom UA string sent by Sync now includes the "oscpu" portion of the default UA string. Eg, with this patch my UA string looks like "Firefox/49.0a1 (Windows NT 6.1; WOW64) FxSync/1.51.0.20160524173017.desktop" where the value in parenthesis is added by this patch. Review commit: https://reviewboard.mozilla.org/r/56442/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/56442/
Attachment #8758069 -
Flags: review?(kcambridge)
Assignee | ||
Comment 6•5 years ago
|
||
The attached patch makes the UA string for me look like "Firefox/49.0a1 (Windows NT 6.1; WOW64) FxSync/1.51.0.20160524173017.desktop". Wes and Danny, can you please formally note that as far as you know, it's fine for me to land this after it gets review and that if anything breaks on the back-end it isn't my fault? ;)
Flags: needinfo?(whd)
Flags: needinfo?(dcoates)
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → markh
Comment 7•5 years ago
|
||
Comment on attachment 8758069 [details] MozReview Request: Bug 1273587 - Change the User-Agent header sent to Sync to include OS information. r?kitcambridge https://reviewboard.mozilla.org/r/56442/#review53256 ::: services/sync/modules/util.js:73 (Diff revision 1) > + _userAgent: null, > + get userAgent() { > + if (!this._userAgent) { > + let hph = Cc["@mozilla.org/network/protocol;1?name=http"].getService(Ci.nsIHttpProtocolHandler); > + this._userAgent = > + Services.appinfo.name + "/" + Services.appinfo.version + // Product. Does it make sense to include the channel, too, or is the version sufficient for that? ::: services/sync/modules/util.js:78 (Diff revision 1) > + Services.appinfo.name + "/" + Services.appinfo.version + // Product. > + " (" + hph.oscpu + ")" + // (oscpu) > + " FxSync/" + WEAVE_VERSION + "." + // Sync. > + Services.appinfo.appBuildID + "."; // Build. > + } > + return this._userAgent + Svc.Prefs.get("client.type", "desktop"); Out of curiosity, why do we use a custom UA string instead of the default Firefox one? Hysterical raisins? ;-)
Attachment #8758069 -
Flags: review?(kcambridge) → review+
Reporter | ||
Comment 8•5 years ago
|
||
Thanks Mark! Nothing I know of will break. The only way I could see this breaking something is if it depends on there *not* being an OS in the parsed string, which I don't think anything does.
Flags: needinfo?(dcoates)
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #7) > Does it make sense to include the channel, too, or is the version sufficient > for that? The default UA doesn't include that - here I was just trying to bring the identical parenthesized version portion from the regular string. Not breaking the server-side parsing is also important. > Out of curiosity, why do we use a custom UA string instead of the default > Firefox one? Hysterical raisins? ;-) rnewman mentioned it was a decision taken long ago, yeah. (In reply to Danny Coates [:dcoates] from comment #8) > Thanks Mark! Nothing I know of will break. The only way I could see this > breaking something is if it depends on there *not* being an OS in the parsed > string, which I don't think anything does. Thanks! I'll give :whd a few days to respond then land it.
Comment 10•5 years ago
|
||
As far as I know, it's fine for you to land this and if anything breaks on the back-end it isn't your fault ;)
Flags: needinfo?(whd)
Comment 11•5 years ago
|
||
Pushed by mhammond@skippinet.com.au: https://hg.mozilla.org/integration/mozilla-inbound/rev/819915e031be Change the User-Agent header sent to Sync to include OS information. r=kitcambridge
Comment 12•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/819915e031be
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in
before you can comment on or make changes to this bug.
Description
•