-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OkHttp migration to instrumentaion API #505
Merged
LikeTheSalad
merged 16 commits into
open-telemetry:main
from
LikeTheSalad:okhttp-migration
Aug 14, 2024
+183
−208
Merged
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
c5c821a
Removing OkHttpInstrumentationConfig statics
LikeTheSalad 690650f
Renaming OkHttpInstrumentationConfig
LikeTheSalad a64e713
Using okhttp instrumentation from registry
LikeTheSalad 655c8b9
Validating install
LikeTheSalad 2ea7107
Updating okhttp instrumentation redme
LikeTheSalad 63bfa3b
Adding autoservice annotation to OkHttpInstrumentation
LikeTheSalad fc17b2b
Removing GlobalOpenTelemetry references from okhttp instrumentation r…
LikeTheSalad 4760178
Merge branch 'refs/heads/main' into okhttp-migration
LikeTheSalad 1b3b6f3
Configuring OkHttp3Singletons from within OkHttpInstrumentation
LikeTheSalad 88c1095
Clean up
LikeTheSalad 80d7c5e
Initializing okhttp initial interceptors to noop
LikeTheSalad c6dde10
Setting up convention for instrumentation tests
LikeTheSalad 660c823
Fixing okhttp tests
LikeTheSalad 78d4593
Clean up
LikeTheSalad 9be6f10
Updating the README
LikeTheSalad b0b2989
Fixing typo
LikeTheSalad File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 0 additions & 20 deletions
20
...io/opentelemetry/instrumentation/library/okhttp/v3_0/OkHttpInstrumentationConfigTest.java
This file was deleted.
Oops, something went wrong.
44 changes: 44 additions & 0 deletions
44
.../java/io/opentelemetry/instrumentation/library/okhttp/v3_0/OkHttpInstrumentationTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* Copyright The OpenTelemetry Authors | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package io.opentelemetry.instrumentation.library.okhttp.v3_0; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.mockito.Mockito.mock; | ||
|
||
import io.opentelemetry.android.OpenTelemetryRum; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class OkHttpInstrumentationTest { | ||
private OkHttpInstrumentation instrumentation; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
instrumentation = new OkHttpInstrumentation(); | ||
} | ||
|
||
@Test | ||
void validateDefaultHttpMethods() { | ||
assertThat(instrumentation.getKnownMethods()) | ||
.containsExactlyInAnyOrder( | ||
"CONNECT", "DELETE", "GET", "HEAD", "OPTIONS", "PATCH", "POST", "PUT", | ||
"TRACE"); | ||
} | ||
|
||
@Test | ||
void validateDefaultRumInstance() { | ||
assertThat(instrumentation.getOpenTelemetryRum()).isEqualTo(OpenTelemetryRum.noop()); | ||
} | ||
|
||
@Test | ||
void validateInstall() { | ||
OpenTelemetryRum rum = mock(); | ||
|
||
instrumentation.install(mock(), rum); | ||
|
||
assertThat(instrumentation.getOpenTelemetryRum()).isEqualTo(rum); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I've applied the changes suggested here taking into account what was discussed in the SIG meeting as well 👍