-
Notifications
You must be signed in to change notification settings - Fork 1k
Move Akka and Armeria virtual fields to helpers #13604
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
Move Akka and Armeria virtual fields to helpers #13604
Conversation
| @SuppressWarnings("unused") | ||
| public static class ConstructorAdvice { | ||
|
|
||
| public static class VirtualFields { |
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.
Until we drop support for inlined advice, both the class and the constant need to be public.
They are accessed directly from the instrumented class, which lives in a different package.
When only indy is support, we can make it a private constant of the advice instead.
| return describeMatch(tree); | ||
| } | ||
|
|
||
| private static boolean isAdviceOrAdviceNestedClass(ClassTree tree, VisitorState state) { |
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.
I'd say this is probably not worth it. Instead of creating the nested VirtualFields class in advice you are better off encapsulating the virtual field access in a util class that internally uses the virtual field. For example in case of ArmeriaServerCallInstrumentation I should have really added an util class in https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/grpc-1.6/library/src/main/java/io/opentelemetry/instrumentation/grpc/v1_6/internal and done the virtual field access there. It is easier to search from ide where the util class is used instead of searching for something like VirtualField.find(ServerCall.class, String.class). If creating a new util class for the virtual fields feels like an overkill we could add the logic to the Singletons class. I think the separate VirtualFields class is fineish in the akka instrumentation for the task context propagation.
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.
Makes sense, fixed in 0f1460d
Part of #13031.
This PR shows with akka as a complex example and armeria as a simple one on which "patterns" I would suggest to use for moving
VirtualField.findcalls from advice classes to constants:VirtualFieldshelper classThis PR is intended as a small starter so that we can discuss the proposed patterns. Based on those, I'll open 2-3 bigger PRs for all remaining usages, luckily there aren't that many.