OkHttp migration to instrumentaion API #505
Conversation
LikeTheSalad
changed the title
| public OpenTelemetryRum getOpenTelemetryRum() { | ||
| return openTelemetryRum; | ||
| } | ||
|
|
||
| @Override | ||
| public void install( | ||
| @NotNull Application application, @NotNull OpenTelemetryRum openTelemetryRum) { | ||
| this.openTelemetryRum = openTelemetryRum; | ||
| } |
There was a problem hiding this comment.
Hmmmm, this feels weird to me. Installing does nothing but hold onto the instance, so that we can add a get() method to this instrumentation so that the lazy singletons can get at the instance. Shouldn't the install() method actually help do the creational work?
The lifecycle is further complicated / obfuscated by the lazy caching stuff in the singletons class.
Here's an idea: What if the install() method just passed the needed otel instance to a static factory method in the singletons class, and that is responsible for setting up the instrumenter and the dependent interceptors?
I made a demonstration PR to this branch to show you more what I'm talking about: LikeTheSalad#1
Note that in addition to the lifecycle being somewhat clearer, it also allows us to remove the otelRum instance, remove the lazy interceptor, and the cached supplier.
There was a problem hiding this comment.
Got it, thanks! I took a quick look and your approach looks a lot cleaner, although I'm concerned about removing the laziness of the singletons due to the timing between the instantiation of OkHttpClient objects and the OTel rum initialization. I remember it was a key issue when using GlobalOpenTelemetry, though maybe it's no longer the case. I'd like to discuss it further in the SIG just to make sure.
There was a problem hiding this comment.
Thanks. I've applied the changes suggested here taking into account what was discussed in the SIG meeting as well 👍
| from | ||
| the [OkHttpInstrumentation](library/src/main/java/io/opentelemetry/instrumentation/library/okhttp/v3_0/OkHttpInstrumentation.java)) | ||
| instance provided via the AndroidInstrumentationLoader as shown below: | ||
|
|
||
| ```java | ||
| OkHttpInstrumentation instrumentation = AndroidInstrumentationLoader.getInstrumentation(OkHttpInstrumentation.class); | ||
|
|
||
| // Call `instrumentation` setters. | ||
| ``` | ||
|
|
||
| > [!NOTE] | ||
| > You must make sure to apply any configurations **before** initializing your OpenTelemetryRum | ||
| > instance (i.e. calling OpenTelemetryRum.builder()...build()). Otherwise your configs won't be | ||
| > taken into account during the RUM initialization process. |
There was a problem hiding this comment.
I like these docs and am wondering how we might document this more broadly in the near future.
There was a problem hiding this comment.
Good point, I've created this issue for it: #532
Migrating the okhttp instrumentation.
This should also address this issue: #433 since now
GlobalOpenTelemetryisn't needed.