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

Instrumentation for redisson 3.17.2 #6096

Merged
merged 4 commits into from
May 26, 2022
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented May 25, 2022

No description provided.

@laurit laurit requested a review from a team as a code owner May 25, 2022 20:50
Comment on lines 9 to 10
versions.set("[3.17.2,)")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add assertInverse and cross-test against the earlier instrumentation?

I'm wondering if the version split will be cleaner at 3.16.8

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially used 3.16.8 and decided to go with 3.17.2 because testing for org.redisson.misc.RPromise seemed better than using an arbitrary class. assertInverse is problematic, apparently it is hard to distinguish 3.x from earlier versions. If I didn't mess anything up then 3.0.0 and 2.5.0 contain exactly the same classes, even all the inner class names match.
Our current approach of detecting version based on the presence or absence of some class resources is slightly flawed. It fails when a child class loader has a different version of the library than what the parent class loader has or if multiple versions of the same library are present in the same class loader.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially used 3.16.8 and decided to go with 3.17.2 because testing for org.redisson.misc.RPromise seemed better than using an arbitrary class

I almost find using RPromise more confusing because it was really dropped from our instrumentation in 3.16.8

how about breaking it on 3.17.0 (which would be a bonus to avoid breaking it on patch version)? looks like RFunction might be a good option for that detection: https://github.com/redisson/redisson/releases/tag/redisson-3.17.0

assertInverse is problematic, apparently it is hard to distinguish 3.x from earlier versions

we could add it to the 3.17 instrumentation module though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, 3.17 is the most reasonable option. Thanks for tracking down RFunction.

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com>
@trask trask merged commit efd40f8 into open-telemetry:main May 26, 2022
@laurit laurit deleted the redisson-3.17.2 branch May 26, 2022 17:34
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.

None yet

2 participants