Add instrumentation for Armeria WebClient. #920
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
trask and
tylerbenson
as code owners
| } | ||
|
|
||
| @Advice.OnMethodExit | ||
| public static void handleSuppression( |
There was a problem hiding this comment.
This craziness is the only pattern I could find for suppressing a non-void method
There was a problem hiding this comment.
Please add some documentation about what you are trying to achieve here
|
|
||
| abstract void configureServer(ServerBuilder sb) | ||
| abstract ServerBuilder configureServer(ServerBuilder serverBuilder) |
There was a problem hiding this comment.
Returning ServerBuilder here allows checking my suppression didn't return null
anuraaga
force-pushed
the
armeria-client-instrumentation
branch
from
4a7a083
to
5d6478f
Compare
| .getClass() | ||
| .getName() | ||
| .equals( | ||
| "io.opentelemetry.instrumentation.armeria.v1_0.server.OpenTelemetryService$Decorator"); |
There was a problem hiding this comment.
i think #892 will break this because it will shade this constant also. there are ways to prevent the string from getting shaded (e.g. make it a constant NAME = "_io.opentelemetry...".substring(1)".
another option for suppressing library instrumentation that might be easier to generalize, is to make the coordination more explicit, e.g. have a method in instrumentation-api like isAutoEnabled(instrumentationName), and the library instrumentation can explicitly check that to see if there is auto instrumentation with the same name enabled
in this case i guess that would mean returning a no-op decorator, or changing the API to expose a method that registers a decorator on the ServerBuilder so we can no-op the registration in this case
I'll add this note to #903, no need to decide/address in this PR
There was a problem hiding this comment.
I ended up needing yet a different pattern because of the shading change making the classnames the same. I guess it's actually a bit more robust now and we'll revisit in #903 for an inversion of control.
| @@ -20,5 +20,6 @@ dependencies { | |||
|
|
|||
| compileOnly group: 'com.linecorp.armeria', name: 'armeria', version: '0.99.8' | |||
|
|
|||
| testImplementation project(path: ':instrumentation:armeria-1.0:library') | |||
| packageName + ".shaded.client.ArmeriaClientTracer$ArmeriaSetter", | ||
| packageName + ".shaded.client.OpenTelemetryClient", | ||
| packageName + ".shaded.client.OpenTelemetryClient$Decorator", | ||
| // .thenAccept(log -> lambda |
There was a problem hiding this comment.
Not sure if this comment is helping or confusing :)
There was a problem hiding this comment.
Tried a bit harder
| } | ||
|
|
||
| @Advice.OnMethodExit | ||
| public static void handleSuppression( |
There was a problem hiding this comment.
Please add some documentation about what you are trying to achieve here
| return "io.opentelemetry.armeria-1.0"; | ||
| } | ||
|
|
||
| private enum ArmeriaSetter implements Setter<ClientRequestContext> { |
There was a problem hiding this comment.
Although this is a nice way to ensure only one instance of the class exists in JVM, it differs from all other instrumentations. Unless there is some strong reason for such differences, I prefer to have uniform codebase.
| }); | ||
| } | ||
|
|
||
| try (Scope ignored = clientTracer.startScope(span, ctx)) { |
There was a problem hiding this comment.
Just to be sure: you start new scope even if span is not recording?
There was a problem hiding this comment.
Yeah - I think it's required for logs injection
There was a problem hiding this comment.
ah, interesting, hadn't thought of that 👍
…-instr-java into armeria-client-instrumentation
* Add instrumentation for Armeria WebClient. * Format: * httpurlconnection * Docs and consistency:
Also updates the server to suppress user-provided decorator when auto instrumenting.
Went ahead and followed the pattern of
GrpcTestby just testing server / client in the same test since it was simple. May extract as I add more sophisticated tests in a future PR (Armeria custom context storage class is coming!)