Record send-tab telemetry for Fenix. #7618
Conversation
| * While this is an async function, it is "fire and forget" - there's no | ||
| * need to await. | ||
| */ | ||
| fun processPendingTelemetry() |
There was a problem hiding this comment.
What if this was named gatherTelemetry and just returned an opaque string or JSON object, instead? Then you'd just pass that along directly to processFxaTelemetry whenever you'd currently call processPendingTelemetry.
If you're doing something that has a by-product of needing to send/set some fxa telemetry, you have an account instance. So you don't need to make any changes to the account manager, you can just directly grab telemetry from the account and pass it along to SyncTelemetry.processFxaTelemetry which knows about what's in the telemetry blob.
Alternatively, SyncTelemetry.processFxaTelemetry can just take an account reference as well, which would make sending a tab something like this:
account.sendSingleTab(targetDeviceId, outgoingCommand.title, outgoingCommand.url)
SyncTelemetry.processFxaTelemetry(account)
We can also rename SyncTelemetry to something more general, perhaps.
There was a problem hiding this comment.
That's much simpler - I failed to see the wood for the trees :) I didn't rename SyncTelemetry but let me know if you would like me to.
...port/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt
Show resolved
Hide resolved
| @@ -284,4 +287,42 @@ object SyncTelemetry { | |||
| val message = reason.message ?: "Unexpected error: ${reason.code}" | |||
| metric.set(message.take(MAX_FAILURE_REASON_LENGTH)) | |||
| } | |||
|
|
|||
| fun processFxaTelemetry(jsonStr: String) { | |||
There was a problem hiding this comment.
Maybe you could link to wherever this schema is defined in the app-services repo? It'll be nice to have that handy here.
...port/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt
Show resolved
Hide resolved
...port/sync-telemetry/src/main/java/mozilla/components/support/sync/telemetry/SyncTelemetry.kt
Outdated
Show resolved
Hide resolved
| import mozilla.components.support.sync.telemetry.GleanMetrics.HistorySync | ||
| import mozilla.components.support.sync.telemetry.GleanMetrics.LoginsSync | ||
| import mozilla.components.support.sync.telemetry.GleanMetrics.Sync | ||
| import mozilla.components.support.sync.telemetry.GleanMetrics.Pings | ||
|
|
||
| const val MAX_FAILURE_REASON_LENGTH = 100 | ||
|
|
||
| // The exceptions we report to the crash reporter, but otherwise don't escape this module. | ||
| sealed class InvalidTelemetryException(cause: Exception) : Exception(cause) { |
There was a problem hiding this comment.
nit: could be internal so that it doesn't into the public API (and linter won't force you to write kdocs).
|
Overall this looks good to me! I guess you should mark it as ready for a review at some point :-) |
Thanks - the app-services part is yet to land :/ Once that does I'll see about making a new release etc and marking this ready. |
|
@mhammond I think this can go ahead now that mozilla/application-services#3308 landed? |
mhammond
changed the title
| @@ -1015,7 +1015,7 @@ open class FxaAccountManager( | |||
|
|
|||
| @VisibleForTesting | |||
| open fun createAccount(config: ServerConfig): OAuthAccount { | |||
| return FirefoxAccount(config) | |||
| return FirefoxAccount(config, null, crashReporter) | |||
There was a problem hiding this comment.
I'm a bit confused by why the PersistCallback is being passed as null here - is there maybe some other creator of a FirefoxAccount which is passed a non-null PersistCallback but will now be getting a null crashReporter?
There was a problem hiding this comment.
as mentioned on slack, this should be fine, just a bad API for FirefoxAccount. It will register a persist callback internally regardless of what you pass in here.
|
That taskcluster failure looks unrelated to this change |
| @@ -111,6 +114,7 @@ class FxaDeviceConstellation( | |||
| when (outgoingCommand) { | |||
| is DeviceCommandOutgoing.SendTab -> { | |||
| account.sendSingleTab(targetDeviceId, outgoingCommand.title, outgoingCommand.url) | |||
| SyncTelemetry.processFxaTelemetry(account.gatherTelemetry()) | |||
There was a problem hiding this comment.
you're not passing in crashReporter to processFxaTelemetry
| @@ -27,7 +28,8 @@ typealias PersistCallback = mozilla.appservices.fxaclient.FirefoxAccount.Persist | |||
| */ | |||
| @Suppress("TooManyFunctions") | |||
| class FirefoxAccount internal constructor( | |||
| private val inner: InternalFxAcct | |||
| private val inner: InternalFxAcct, | |||
| private val crashReporter: CrashReporting? = null | |||
There was a problem hiding this comment.
did you mean to use this for the FxaDeviceConstellation constructor?
| @@ -1015,7 +1015,7 @@ open class FxaAccountManager( | |||
|
|
|||
| @VisibleForTesting | |||
| open fun createAccount(config: ServerConfig): OAuthAccount { | |||
| return FirefoxAccount(config) | |||
| return FirefoxAccount(config, null, crashReporter) | |||
There was a problem hiding this comment.
as mentioned on slack, this should be fine, just a bad API for FirefoxAccount. It will register a persist callback internally regardless of what you pass in here.
application-services needs to be closely involved in creating the data to be recorded in telemetry, which passes it back to Android Components as a JSON string. SyncTelemetry.kt is the only thing that needs to know what's in that JSON, so it pull it apart and calls the relevant glean functions. See also mozilla/application-services#3308
|
bors r+ |
7618: Record send-tab telemetry for Fenix. r=grigoryk a=mhammond application-services needs to be closely involved in creating the data to be recorded in telemetry - however, we are trying to avoid exposing this telemetry directly to the android-components "concepts" or "features" - for example, the part of android-components that sends a tab ideally wouldn't have to deal with any interface changes just because telemetry is being sent or changed - otherwise new/changed telemetry is always a "breaking change" In the future, we expect the rust components to interact directly with Glean, and consumers like android-components wouldn't need to get involved at all. However, until then... The idea with this PR is that there's one new public account function, `processPendingTelemetry()`, which should be called whenever something is done which might record telemetry. However, it doesn't need to know what is actually recorded - that knowledge is only in the telemetry code - the code that is tightly bound to glean. This hasn't been tested yet - but it builds :) I'm soliciting feedback on the general shape of this before I invest too much more in tests etc. I also considered trying to do with Observers(), but that didn't seem any easier. I'm obviously open to alternative approaches! See also mozilla/application-services#3308 Co-authored-by: Mark Hammond <[email protected]>
|
Timed out. |
|
bors retry |
|
Build succeeded: |
application-services needs to be closely involved in creating the
data to be recorded in telemetry - however, we are trying to avoid
exposing this telemetry directly to the android-components "concepts"
or "features" - for example, the part of android-components that sends
a tab ideally wouldn't have to deal with any interface changes just because
telemetry is being sent or changed - otherwise new/changed telemetry is always
a "breaking change"
In the future, we expect the rust components to interact directly with Glean,
and consumers like android-components wouldn't need to get involved at all.
However, until then...
The idea with this PR is that there's one new public account function,
processPendingTelemetry(), which should be called whenever something isdone which might record telemetry. However, it doesn't need to know
what is actually recorded - that knowledge is only in the telemetry
code - the code that is tightly bound to glean.
This hasn't been tested yet - but it builds :) I'm soliciting feedback on
the general shape of this before I invest too much more in tests etc.
I also considered trying to do with Observers(), but that didn't seem
any easier. I'm obviously open to alternative approaches!
See also mozilla/application-services#3308
CC @grigoryk @linacambridge @rfk
Pull Request checklist
After merge