Closed Bug 1273587 Opened 3 years ago Closed 3 years ago

Add OS to Sync user agent string

Categories

(Firefox :: Sync, defect, P1)

defect

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
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
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.
Adding :whd for awareness re: potential changes to this string
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™.
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)
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: nobody → markh
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+
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)
(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.
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)
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
https://hg.mozilla.org/mozilla-central/rev/819915e031be
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.