-
Notifications
You must be signed in to change notification settings - Fork 1k
Add NATS instrumentation #13999
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
Add NATS instrumentation #13999
Conversation
🔧 The result from spotlessApply was committed to the PR branch. |
@laurit is it you I should ask a review from? |
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.
Currently there is a build failure, and a test failure.
.../io/opentelemetry/javaagent/instrumentation/nats/v2_21/ConnectionRequestInstrumentation.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/nats/v2_21/NatsInstrumentationModule.java
Outdated
Show resolved
Hide resolved
...in/java/io/opentelemetry/javaagent/instrumentation/nats/v2_21/NatsInstrumentationModule.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/javaagent/instrumentation/nats/v2_21/SubscriptionInstrumentation.java
Outdated
Show resolved
Hide resolved
...ts-2.21/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_21/NatsTelemetry.java
Outdated
Show resolved
Hide resolved
...brary/src/main/java/io/opentelemetry/instrumentation/nats/v2_21/OpenTelemetryConnection.java
Outdated
Show resolved
Hide resolved
.../main/java/io/opentelemetry/instrumentation/nats/v2_21/internal/NatsInstrumenterFactory.java
Outdated
Show resolved
Hide resolved
.../java/io/opentelemetry/instrumentation/nats/v2_21/internal/NatsRequestContextCustomizer.java
Outdated
Show resolved
Hide resolved
...y/src/test/java/io/opentelemetry/instrumentation/nats/v2_21/NatsTelemetryDispatcherTest.java
Outdated
Show resolved
Hide resolved
fcb32fb
to
487dffb
Compare
...y/src/main/java/io/opentelemetry/instrumentation/nats/v2_21/OpenTelemetryMessageHandler.java
Outdated
Show resolved
Hide resolved
40e1913
to
7d1b355
Compare
89cd845
to
360a21f
Compare
} | ||
|
||
/** Returns a {@link Options.Builder} with instrumented {@link DispatcherFactory}. */ | ||
public Options.Builder wrap(Options.Builder options) { |
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.
not really sure about this, as I have to rely on a build
. I think it would be better to let users do options.dispatcherFactory(...)
as OpenTelemetryDispatcherFactory
is now exposed
hey @laurit could you have a second look at this one when you have the time? muzzle passes locally and on previous builds, so I guess the error is transient. |
might be worth checking if this needs to be applied before merging |
.../library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetryBuilder.java
Show resolved
Hide resolved
.../library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetryBuilder.java
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Outdated
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Outdated
Show resolved
Hide resolved
...ts-2.17/library/src/main/java/io/opentelemetry/instrumentation/nats/v2_17/NatsTelemetry.java
Outdated
Show resolved
Hide resolved
...test/java/io/opentelemetry/instrumentation/nats/v2_17/NatsInstrumentationDispatcherTest.java
Outdated
Show resolved
Hide resolved
assertThatNoException() | ||
.isThrownBy( | ||
() -> { | ||
d2.unsubscribe(s1); | ||
d2.unsubscribe(s2); | ||
connection.closeDispatcher(d1); | ||
connection.closeDispatcher(d2); | ||
}); | ||
} |
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.
one way we use for cleanup is
Line 52 in 5b75b3f
@RegisterExtension static final AutoCleanupExtension cleanup = AutoCleanupExtension.create(); |
you can call
cleanup.deferCleanup(...);
immediately after you create whatever needs to be closed/cleaned and it will get executed after the test event when the test fails
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.
deferCleanup requires AutoCloseable
which is not the case for Dispatcher
or Subscription
.
Also this is to test properly this proxy
// public void closeDispatcher(Dispatcher dispatcher)
private Object closeDispatcher(Method method, Object[] args) throws Throwable {
if (method.getParameterCount() == 1
&& args[0] instanceof Proxy
&& Proxy.getInvocationHandler(args[0]) instanceof OpenTelemetryDispatcher) {
args[0] = ((OpenTelemetryDispatcher) Proxy.getInvocationHandler(args[0])).getDelegate();
}
return invokeMethod(method, delegate, args);
}
Will the cleanup propagate the error to the test in case this is failing? Because this means that we didn't "unwrap" the otel wrapper, which would fail at runtime
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.
as per the intended cleanup, it's declared in the abstract parent class
@AfterAll
static void afterAll() throws InterruptedException, TimeoutException {
connection.drain(Duration.ZERO);
natsContainer.close();
}
...a/io/opentelemetry/instrumentation/nats/v2_17/AbstractNatsInstrumentationDispatcherTest.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/instrumentation/nats/v2_17/AbstractNatsInstrumentationDispatcherTest.java
Outdated
Show resolved
Hide resolved
...a/io/opentelemetry/instrumentation/nats/v2_17/AbstractNatsInstrumentationDispatcherTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
public static void onExit(@Advice.Enter Message message) {} |
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.
You should assign message
to the message return value otherwise this method will end up returning null
which is not what it should be doing. You can verify this by modifying your test to assert on the return value of this method.
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.
totally forgot to test the return type of the Request methods. Thanks for the reminder!
@laurit thanks a lot for your time on this |
Co-authored-by: Jay DeLuca <[email protected]>
Co-authored-by: otelbot <[email protected]> Co-authored-by: Lauri Tulmin <[email protected]> Co-authored-by: Jay DeLuca <[email protected]>
First, sorry for the review of 5000 rows, that's not very nice.
This PR aims at instrumenting NATS for both library and java agent, using the messaging conventions.
Few things: