Closed Bug 1014957 Opened 8 years ago Closed 8 years ago

Add telemetry probes for self-hosted Sync usage

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 32

People

(Reporter: ckarlof, Assigned: markh)

References

Details

(Whiteboard: p=1 s=it-32c-31a-30b.3 [qa-])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 999628
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.
Attachment #8427470 - Flags: feedback?(ckarlof)
Whiteboard: p=1 s=it-32c-31a-30b.2 [qa-]
Added to Iteration 32.2
Assignee: nobody → mhammond
Status: NEW → ASSIGNED
Whiteboard: p=1 s=it-32c-31a-30b.2 [qa-] → p=1 s=it-32c-31a-30b.3 [qa-]
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!
Attachment #8427470 - Flags: feedback?(ckarlof) → feedback+
Attachment #8427470 - Flags: review?(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.
Attachment #8427470 - Flags: review?(rnewman) → review+
(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.
Attachment #8427470 - Attachment is obsolete: true
Attachment #8429798 - Flags: review?(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?
Attachment #8429798 - Flags: review?(rnewman) → review+
(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?
Flags: needinfo?(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",
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/58b2af3a5562
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.