NO-TICKET: Upgrade Otel BOM dependency - #1683
Conversation
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d44e5159f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 447d205d1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| INSTRUMENTATION_NAME, | ||
| httpSpanNameExtractor | ||
| ) | ||
| .setInstrumentationVersion(BuildConfig.VERSION_NAME) |
There was a problem hiding this comment.
Question - We've not been setting the instrumentationScopeInfo.version for any of our instrumentations (as it is same as rum.sdk.version) and just setting the instrumentationScopeInfo.name. Earlier http instrumentations were used from upstream so a instrumentationScopeInfo.version did help but may be not needed / redundant now?
There was a problem hiding this comment.
@aditi-s3 Did you decide to keep this redundant attribute?
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaa9fedc8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a1e679e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae04569fce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd1ebccc87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95d951ec1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| log.attributes.asMap().forEach attrs@{ (key, value) -> | ||
| if (key.key == GlobalRumConstants.LOG_EVENT_NAME_KEY.key) { |
There was a problem hiding this comment.
If we're preserving the fallback to event.name above, we need to preserve this block here to ensure event.name attribute is not added to the span as it's job is already done above - i.e supplying the span.name
But a neater solution is to remove line 98 above as we're no longer using event.name attribute anywhere now. Also remove the GlobalRumConstant LOG_EVENT_NAME_KEY as it won't be used anymore.
There was a problem hiding this comment.
I had the neater solution initially but Codex review comment #1683 (comment) prompted me to change it back. But yes added the block to skip adding it as a span attribute.
| } | ||
|
|
||
| @Test | ||
| fun `a custom extractor cannot override the crash event name`() { |
There was a problem hiding this comment.
This test is no longer needed and can be removed.
| } | ||
| } | ||
|
|
||
| // HttpUrlConnectionSingletons and OkHttpSingletons disable the instrumenter SPI lookup |
There was a problem hiding this comment.
nit: Remove this comment. It's confusing here. You already added comments where you suppressed the SPI lookup.
There was a problem hiding this comment.
It was intentional to indicate why we are not doing a centralized override. But removed it
|
|
||
| /** Extracts the configured `peer.service` mapping for auto-instrumented OkHttp requests. */ | ||
| internal class PeerServiceAttributesExtractor( | ||
| private val attributesGetter: ServerAttributesGetter<Interceptor.Chain>, |
There was a problem hiding this comment.
Why not use the super type :
| private val attributesGetter: ServerAttributesGetter<Interceptor.Chain>, | |
| private val attributesGetter: HttpClientAttributesGetter<Interceptor.Chain, Response>, |
And also get the path on line 52 using the following instead of passing null:
attributesGetter.getUrlFull(request)
|
|
||
| val instrumenter: Instrumenter<Interceptor.Chain, Response> = instrumenterBuilder.build() | ||
| // Avoid the instrumenter SPI lookup's one-time disk read, which trips Android StrictMode | ||
| // (see open-telemetry/opentelemetry-java-instrumentation#19954). |
There was a problem hiding this comment.
Added word "issue" to the comments. You are searching PRs in the screenshot so no results were returned
| // Null unless the caller opts in via setSpanNameExtractor(). | ||
| private Function<SpanNameExtractor<Interceptor.Chain>, SpanNameExtractor<Interceptor.Chain>> | ||
| spanNameExtractorTransformer; | ||
| private Map<String, String> peerServiceMapping = java.util.Collections.emptyMap(); |
There was a problem hiding this comment.
| private Map<String, String> peerServiceMapping = java.util.Collections.emptyMap(); | |
| private Map<String, String> peerServiceMapping = Collections.emptyMap(); |
| * Configures the extractor of the {@code peer.service} span attribute. | ||
| */ | ||
| public OkHttpTelemetryBuilder setPeerServiceMapping(Map<String, String> peerServiceMapping) { | ||
| this.peerServiceMapping = new java.util.HashMap<>(peerServiceMapping); |
There was a problem hiding this comment.
| this.peerServiceMapping = new java.util.HashMap<>(peerServiceMapping); | |
| this.peerServiceMapping = HashMap<>(peerServiceMapping); |
| OkHttpClientInstrumenterBuilderFactory.create(openTelemetry); | ||
| builder.addAttributesExtractor( | ||
| new PeerServiceAttributesExtractor( | ||
| com.splunk.rum.instrumentation.okhttp3.common.internal.OkHttpAttributesGetter.INSTANCE, |
There was a problem hiding this comment.
| com.splunk.rum.instrumentation.okhttp3.common.internal.OkHttpAttributesGetter.INSTANCE, | |
| OkHttpAttributesGetter.INSTANCE, |
| try { | ||
| builder.buildClientInstrumenter(RequestPropertySetter) | ||
| } finally { | ||
| ServiceLoaderUtil.setLoadFunction { serviceType -> ServiceLoader.load(serviceType) } |
There was a problem hiding this comment.
Not sure this can actually restore anything. ServiceLoaderUtil has no getter, only load and
setLoadFunction so we just overwrite with a fresh ServiceLoader::load. If a host app or
another OTel library had its own load function then I think its gone permanently.
Maybe can consider setting the override once, and gate it on a ThreadLocal so only our building thread sees the empty list. That closes the concurrent-build hole and clobbers the host once instead of every
build. Might be worth asking for a getter on #19954.
Also, lines 89 and 93 call into io.opentelemetry.instrumentation.api.internal ,
which is marked "This class is internal and is hence not for public use. Its APIs are
unstable and can change at any time"
We export the BOM via api(platform(...)), so I worry that a host forcing a different
opentelemetry-instrumentation-api can hypothetically make setLoadFunction vanish: NoSuchMethodError out of install() on the main thread, i.e. the customer's app won't start.
Maybe we should catch LinkageError and then fall back to a normal build turns that into a StrictMode warning. This is the one thing e2e testing may not cover, since e2e ran against our own dependency graph.
There was a problem hiding this comment.
Updated comment to clarify there is no restoration
@tonzhan2 I am not inclined to increase the complexity of a temporary workaround with ThreadLocal. Limiting the window of override is okay I think.
I took your recommendation to catch errors for when setLoadFunction is unavailable in case of a competing OTel dependency.
| // Isolated so Function#apply (requires API 24 or desugaring) only runs when a caller opts in | ||
| // via setSpanNameExtractor(), not on every build() call. | ||
| @SuppressLint("NewApi") // This existing java.util.function API requires consumer desugaring. |
There was a problem hiding this comment.
In our SDK we use a simple comment :
| // Isolated so Function#apply (requires API 24 or desugaring) only runs when a caller opts in | |
| // via setSpanNameExtractor(), not on every build() call. | |
| @SuppressLint("NewApi") // This existing java.util.function API requires consumer desugaring. | |
| @SuppressLint("NewApi") // Requires API 24 or core library desugaring |
| try { | ||
| instrumenterBuilder.build() | ||
| } finally { | ||
| ServiceLoaderUtil.setLoadFunction { serviceType -> ServiceLoader.load(serviceType) } |
There was a problem hiding this comment.
Same thing as my comment on HttpUrlConnectionSingletons, maybe catch a linkageError then fall back to a normal build turns that into a StrictMode warning.
| try { | ||
| return builder.build(); | ||
| } finally { | ||
| ServiceLoaderUtil.setLoadFunction(ServiceLoader::load); |
There was a problem hiding this comment.
Same thing as my comment on HttpUrlConnectionSingletons, maybe catch a linkageError then fall back to a normal build turns that into a StrictMode warning.
| try { | ||
| builder.buildClientInstrumenter(RequestPropertySetter) | ||
| } finally { | ||
| ServiceLoaderUtil.setLoadFunction { serviceType -> ServiceLoader.load(serviceType) } |
There was a problem hiding this comment.
Question - Are we sure that this restores the behavior? Do we need to restore it?
There was a problem hiding this comment.
It does not - updated the comments.

Title: Upgrade OTel dependencies
Description
newPeerServiceResolver()API as it was not exposed via module configuration anywayChecklist
Generative AI usage
How to Test These Changes
Launch a sample app with the latest changes and test
Future Considerations (Optional)
Need to track OTel updates and pro-actively identify risks/updates that should be included. Long term consider moving to Kotlin SDK ?