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

Actually fix redisson instrumentation after 3.16.8 release #5201

Merged

Conversation

mateuszrzeszutek
Copy link
Member

Implemented Lauri's idea of using VirtualFields to avoid calling getPromise()

@mateuszrzeszutek mateuszrzeszutek requested a review from a team January 21, 2022 17:29
Copy link
Contributor

@breedx-splk breedx-splk left a comment

Choose a reason for hiding this comment

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

Looks good to me. It took me a bit to figure out the CommandData vs. CommandsData difference. Does it make sense to combine those into a single class that takes the class and name? Might be a little clearer. 🤷🏻

@mateuszrzeszutek
Copy link
Member Author

You still need to have different advice classes for these two - except the method matcher there's pretty much nothing in common.

@breedx-splk
Copy link
Contributor

You still need to have different advice classes for these two - except the method matcher there's pretty much nothing in common.

Ah yeah, didn't consider that we still need two advices because we don't control creation of them (and just pass the name as a string). Gotcha.

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.

what do you think about using reflection to call getPromise() instead?

@mateuszrzeszutek mateuszrzeszutek force-pushed the actually-fix-redisson-3.16.8 branch from 015244e to 0c36f17 Compare January 24, 2022 08:37
@mateuszrzeszutek
Copy link
Member Author

what do you think about using reflection to call getPromise() instead?

That could work too! I reworked this PR to use MethodHandles instead

@trask trask merged commit 064d923 into open-telemetry:main Jan 24, 2022
@mateuszrzeszutek mateuszrzeszutek deleted the actually-fix-redisson-3.16.8 branch January 25, 2022 06:58
RashmiRam pushed a commit to RashmiRam/opentelemetry-auto-instr-java that referenced this pull request May 23, 2022
…metry#5201)

* Actually fix redisson instrumentation after 3.16.8 release

* try MethodHandles

* remove unneeded virtual fields
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.

5 participants