1273587 - Add OS to Sync user agent string

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Danny Coates [:dcoates]]

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

Comment 1

[Mark Hammond [:markh]]

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?

Comment 2

[Ryan Kelly [:rfkelly]]

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

[Ryan Kelly [:rfkelly]]

Adding :whd for awareness re: potential changes to this string

Comment 4

[Danny Coates [:dcoates]]

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

Comment 5

[Mark Hammond [:markh]]

Attachment: MozReview Request: Bug 1273587 - Change the User-Agent header sent to Sync to include OS information. r?kitcambridge

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/

Comment 6

[Mark Hammond [:markh]]

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? ;)

Comment 7

[Lina Cambridge (she/her) [:lina]]

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? ;-)

Comment 8

[Danny Coates [:dcoates]]

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.

Comment 9

[Mark Hammond [:markh]]

(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

[Wesley Dawson [:whd]]

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 ;)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/819915e031be