1014957 - Add telemetry probes for self-hosted Sync usage

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Chris Karlof [:ckarlof]]

Because we'd like to know how many Sync users self-host. For FxA Sync users that might be a non-default tokenServer or FxA URL. For old Sync users that might be a  non-default serverURL.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1014957-add-telemetry-probe-to-indicate-if-a-cus.patch

I took the low-road here - just a list of prefs which covers *both* legacy and fxa, with a single probe that records if any of those prefs have non-default values.

Comment 2

[Marco Mucci [:MarcoM]]

Added to Iteration 32.2

Comment 3

[Chris Karlof [:ckarlof]]

Comment on attachment 8427470 [details] [diff] [review]
0003-Bug-1014957-add-telemetry-probe-to-indicate-if-a-cus.patch

Review of attachment 8427470 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8427470 [details] [diff] [review]
0003-Bug-1014957-add-telemetry-probe-to-indicate-if-a-cus.patch

Review of attachment 8427470 [details] [diff] [review]:
-----------------------------------------------------------------

r+ because the code is fine, modulo typographic nits.

But I encourage you to spend the minor cost of an additional histogram to get better granularity. After all, we already have hundreds of bullshit probes for minor potential perf issues; this is a real product direction compass, so let's get the best info we can!

::: services/sync/modules/service.js
@@ +54,5 @@
> +// A list of preference names which are checked for user-modified values and
> +// reflected into a telemetry probe.
> +const TELEMETRY_CUSTOM_SERVER_PREFS = [
> +  "services.sync.serverURL",
> +  "services.sync.jpake.serverURL",

I'm in two minds about whether to include the keyexchange server URL here.

Consider two points:

1. If you use your own J-PAKE server to pair, but you're syncing to a Mozilla server, do we care?
2. Probably nobody has ever done this (there was never UI to do so, and I imagine the paranoid folks would type in credentials rather than pairing). But if I'm wrong about that, it'll throw the numbers off (see #1).

So I'd be inclined to remove that pref from this list.

@@ +56,5 @@
> +const TELEMETRY_CUSTOM_SERVER_PREFS = [
> +  "services.sync.serverURL",
> +  "services.sync.jpake.serverURL",
> +  "services.sync.tokenServerURI",
> +  "identity.fxaccounts.auth.uri",

I think we'd like more granularity here: that is, we would like to distinguish between users running their own FxA setup and users who are running only their own Sync storage nodes.

::: toolkit/components/telemetry/Histograms.json
@@ +6012,5 @@
> +  },
> +  "WEAVE_CUSTOM_SERVER_CONFIGURATION": {
> +    "expires_in_version": "never",
> +    "kind": "boolean",
> +    "description": "Whether sync is configured to use a custom server"

Whether Sync or FxA are configured to use custom servers.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: With suggested changes

(In reply to Richard Newman [:rnewman] from comment #4)

> But I encourage you to spend the minor cost of an additional histogram to
> get better granularity. After all, we already have hundreds of bullshit
> probes for minor potential perf issues; this is a real product direction
> compass, so let's get the best info we can!

Yeah - agreed.

> I'm in two minds about whether to include the keyexchange server URL here.

I'll defer to your greater understanding of the issues here - so I removed that.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8429798 [details] [diff] [review]
With suggested changes

Review of attachment 8429798 [details] [diff] [review]:
-----------------------------------------------------------------

No need for re-re-review.

::: services/sync/modules/service.js
@@ +51,5 @@
>                              INFO_COLLECTION_COUNTS,
>                              INFO_QUOTA];
>  
> +// A structure recording a list of preference names, keyed by the name of
> +// a (boolean) telemetry probe.  The probe will record true of any of the prefs

s/of/if

@@ +59,5 @@
> +    "services.sync.serverURL",
> +  ],
> +  WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: [
> +    "identity.fxaccounts.auth.uri",
> +    "services.sync.tokenServerURI",

Let's even split this: WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION. That's what I meant by running just storage versus the full stack. You can use a custom token server (and thus 1.5 server) with Mozilla's FxA auth URI, right?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Richard Newman [:rnewman] from comment #6)
> @@ +59,5 @@
> > +    "services.sync.serverURL",
> > +  ],
> > +  WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: [
> > +    "identity.fxaccounts.auth.uri",
> > +    "services.sync.tokenServerURI",
> 
> Let's even split this: WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION. That's what
> I meant by running just storage versus the full stack. You can use a custom
> token server (and thus 1.5 server) with Mozilla's FxA auth URI, right?

To be clear, you are asking me to rename the probe and remove the .auth.uri pref from the array?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Mark Hammond [:markh] from comment #7)

Sorry for being unclear.

> To be clear, you are asking me to rename the probe and remove the .auth.uri
> pref from the array?

Three probes, three prefs.

// Legacy...
WEAVE_CUSTOM_LEGACY_SERVER_CONFIGURATION: "services.sync.serverURL",

// or one or both of these:
WEAVE_CUSTOM_FXA_SERVER_CONFIGURATION: "identity.fxaccounts.auth.uri",
WEAVE_CUSTOM_TOKEN_SERVER_CONFIGURATION: "services.sync.tokenServerURI",

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Ah yes - thanks!

https://hg.mozilla.org/integration/fx-team/rev/58b2af3a5562

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/58b2af3a5562