Instrumentation api prototype #305
Conversation
| } | ||
| } | ||
|
|
||
| fun apply( |
There was a problem hiding this comment.
I think apply is a little too vague/generic. To a novice, it could be unclear if apply() is/should-be called more than once, like maybe each time you call apply() it generates data or something.
I think a verb like install() could help things be a little clearer.
| private var screenNameExtractor: ScreenNameExtractor = ScreenNameExtractor.DEFAULT | ||
| private var tracerCustomizer: (Tracer) -> Tracer = { it } | ||
|
|
||
| fun setTracerCustomizer(customizer: (Tracer) -> Tracer) { | ||
| tracerCustomizer = customizer | ||
| } | ||
|
|
||
| fun setScreenNameExtractor(screenNameExtractor: ScreenNameExtractor) { | ||
| this.screenNameExtractor = screenNameExtractor | ||
| } |
There was a problem hiding this comment.
I think it's totally fine for the purposes of a prototype, but I think we should be favoring immutability, and these should be injected rather than .set().
There was a problem hiding this comment.
...and I'm not just being dogmatic. I think the setters can give the user a sense that they can change the instrumentation at runtime (although they probably won't have access to it anyway).
|
|
||
| AnrDetectorToggler listener = new AnrDetectorToggler(anrWatcher, scheduler); | ||
| // call it manually the first time to enable the ANR detection | ||
| listener.onApplicationForegrounded(); | ||
|
|
||
| instrumentedApplication.registerApplicationStateListener(listener); | ||
| ServiceManager.get().getService(AppLifecycleService.class).registerListener(listener); |
There was a problem hiding this comment.
I've been bitten by the service locator antipattern so so many times. I would love to discuss ways of trying to avoid doing this.
There was a problem hiding this comment.
Given that we have a means of identifying Services via the interface and determining if each should be on, is there a more explicit way of registering them in a shared way so that the registry is exposed for consumption scenarios like this, but it'll just be a simple map lookup?
Another thought: are any instrumentations and services able to access any registered service? That seems a bit chaotic, especially if certain services are optional, so we'll effectively have another dependency graph on our hands that isn't explicitly managed. Is there a way this can be closed off, or otherwise make them part of the interface the Agent exposes so they can be referenced explicitly rather than looked up?
There was a problem hiding this comment.
effectively have another dependency graph on our hands that isn't explicitly managed
Yeah this is exactly the main issue! And it's global/static and very difficult to unweave later.
| import java.util.List; | ||
|
|
||
| @AutoService(AndroidInstrumentation.class) | ||
| public final class NetworkChangeMonitorInstrumentation implements AndroidInstrumentation { |
There was a problem hiding this comment.
This reads a little clunky now -- I think something just like NetworkInstrumentation is sufficient, or even NetworkChangeInstrumentation. The "monitor" part of it was supposed to represent something that is looking out for network changes. Maybe a little pedantic, but this instrumentation isn't for the monitor.
| * time. | ||
| */ | ||
| @AutoService(AndroidInstrumentation.class) | ||
| public final class SlowRenderingDetectorInstrumentation implements AndroidInstrumentation { |
There was a problem hiding this comment.
Similar to the network, this simply becomes SlowRenderingInstrumentation or SlowRenderInstrumentation.
| import io.opentelemetry.android.OpenTelemetryRum | ||
|
|
||
| interface AndroidInstrumentation { | ||
| companion object { |
There was a problem hiding this comment.
Stylistically, since they are working in concert, it may be better to put related singletons in a regular object rather than a companion object on each of the interfaces? (Comment applies to other similar changes too)
| import android.app.Application | ||
| import io.opentelemetry.android.OpenTelemetryRum | ||
|
|
||
| interface AndroidInstrumentation { |
There was a problem hiding this comment.
Just to confirm, the idea is that each AndroidInstrumentation works independently, and they only need to interact with OpenTelemetryRum to access the OTel SDK bits (e.g. Tracer)? Will the Agent ever need to reference the Instrumentations at all, for validations, throttling, or other needs?
There was a problem hiding this comment.
Just to confirm, the idea is that each AndroidInstrumentation works independently, and they only need to interact with OpenTelemetryRum to access the OTel SDK bits (e.g. Tracer)?
I don't remember if we discussed this in the SIG, but yes, this is correct.
Will the Agent ever need to reference the Instrumentations at all, for validations, throttling, or other needs?
Right now that's not the case, the agent doesn't need to do anything else with the instrumentations other than "activating" them by calling AndroidInstrumentation.apply (or install, depending on what name we decide to use) to provide them with the rum object plus the Application object. Though this can be changed if needed, I just closed this PR to start working on the changes in smaller PRs where we could discuss it all more in detail.
|
|
||
| AnrDetectorToggler listener = new AnrDetectorToggler(anrWatcher, scheduler); | ||
| // call it manually the first time to enable the ANR detection | ||
| listener.onApplicationForegrounded(); | ||
|
|
||
| instrumentedApplication.registerApplicationStateListener(listener); | ||
| ServiceManager.get().getService(AppLifecycleService.class).registerListener(listener); |
There was a problem hiding this comment.
Given that we have a means of identifying Services via the interface and determining if each should be on, is there a more explicit way of registering them in a shared way so that the registry is exposed for consumption scenarios like this, but it'll just be a simple map lookup?
Another thought: are any instrumentations and services able to access any registered service? That seems a bit chaotic, especially if certain services are optional, so we'll effectively have another dependency graph on our hands that isn't explicitly managed. Is there a way this can be closed off, or otherwise make them part of the interface the Agent exposes so they can be referenced explicitly rather than looked up?
|
Will close this as it will get implemented in other PRs. |
Related to this issue.
POC display. Not meant to be merged. The tests haven't been updated for this poc so the Github checks are not supposed to pass. The purpose of this draft is to provide a general view of what an instrumentation API would look like and what it would take to implement it in the existing instrumentation modules.
These slides show what it's meant to be changed by this POC: https://docs.google.com/presentation/d/1ftU8GgdulR2_kHHTSCr6j5x1_dH4XotTq_PXNo5bzx4/edit?usp=sharing