1370656 - Same instance of TelemetryCollector is being re-used between syncs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement)

Description

[:Grisha Kruglov]

We have a static reference to the sync adapter in FxAccountSyncService, and since TelemetryCollector is a field in the sync adapter, and it's never cleared, we'll end up re-using whatever contents it had from previous syncs that were not overridden.

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1370656 - Don't re-use the same instance of TelemetryCollector between syncs

Parent class (FxAccountSyncAdapter) is essentially a singleton, and so we'd end up re-using class
fields between syncs, among them the collected telemetry data. It's cleaner and safer to move
ownership of TelemetryCollector into IntrumentedSessionCallback. With this change, telemetry
data is contained within and eventually emitted from a single owner object.

Review commit: https://reviewboard.mozilla.org/r/146354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/146354/

Comment 2

[:Grisha Kruglov]

Comment on attachment 8875012 [details]
Bug 1370656 - Don't re-use the same instance of TelemetryCollector between syncs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146354/diff/1-2/

Comment 3

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8875012 [details]
Bug 1370656 - Don't re-use the same instance of TelemetryCollector between syncs

https://reviewboard.mozilla.org/r/146354/#review150384

The next step on this road is to make the `GlobalSession` own the `TelemetryCollector`... except you don't want that since you want to capture information before the global session starts.  Right?

Comment 4

[:Grisha Kruglov]

(In reply to Nick Alexander :nalexander from comment #3)
> Comment on attachment 8875012 [details]
> Bug 1370656 - Don't re-use the same instance of TelemetryCollector between
> syncs
> 
> https://reviewboard.mozilla.org/r/146354/#review150384
> 
> The next step on this road is to make the `GlobalSession` own the
> `TelemetryCollector`... except you don't want that since you want to capture
> information before the global session starts.  Right?

That's correct. I've started out with that approach, but ended up moving the ownership out of the session object for that reason.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09c76ee950d0
Don't re-use the same instance of TelemetryCollector between syncs r=nalexander

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09c76ee950d0