Skip to content
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

remove proguard config for InstrumenationUtil #529

Merged

Conversation

SylvainJuge
Copy link
Contributor

With SDK 1.41.0 io.opentelemetry.exporter.internal.InstrumentationUtil is now deprecated and replaced with io.opentelemetry.api.internal.InstrumentationUtil directly in the API.

Update to the 1.41.0 SDK has been done in #521 and is now merged.

As a consequence, the proguard configuration is no more needed for this class.

@LikeTheSalad can you tell me if there is anything related to InstrumentationUtil usage that needs to be updated here ?

@SylvainJuge SylvainJuge requested a review from a team August 12, 2024 15:56
@LikeTheSalad
Copy link
Contributor

LikeTheSalad commented Aug 13, 2024

Hi! Thank you for bringing this up, the proguard rule removed in this PR has 2 dependencies, one is the otel java API, which is now fine, and the other one is the otel java instrumentation API. So we should technically be able to remove this rule after this other PR is merged and a new version of the instrumentation API is released and used here. This is because the rule keep class io.opentelemetry.exporter.internal.InstrumentationUtil ensures that said class name isn't changed during obfuscation, so that the reflection used here doesn't get affected.

However, it seems like we're still planning to use reflection in the java instrumentation lib even after the otel java changes. If that PR gets merged as is, then instead of removing the rule keep class io.opentelemetry.exporter.internal.InstrumentationUtil, we would instead have to change the rule to target the new reflection, i.e. io.opentelemetry.api.internal.InstrumentationUtil.

I took a better look and it seems like the reflection used there shouldn't affect Android as it's only used to ensure that the new class is available in the classpath. Though let me take a deeper look just in case it might still affect the way this should work in Android.

@LikeTheSalad
Copy link
Contributor

I've added a comment here because I think the new reflection might affect this functionality for Android unless we modify our rule.

@SylvainJuge
Copy link
Contributor Author

Usage of reflection was removed from open-telemetry/opentelemetry-java-instrumentation#11985 and it's now merged, so I think we should be good to merge it as-is.

@LikeTheSalad
Copy link
Contributor

Usage of reflection was removed from open-telemetry/opentelemetry-java-instrumentation#11985 and it's now merged, so I think we should be good to merge it as-is.

Yep, we can merge this change as-is since they've removed reflection. I just think we should wait for a new version of the instrumentation API to be created with those changes and we should be good to go then.

@LikeTheSalad
Copy link
Contributor

It's done now. Cheers!

@LikeTheSalad LikeTheSalad merged commit 6f4bcc3 into open-telemetry:main Aug 19, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants