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? |
laurit
left a comment
There was a problem hiding this comment.
Currently there is a build failure, and a test failure.
fcb32fb to
487dffb
Compare
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.
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 |
| assertThatNoException() | ||
| .isThrownBy( | ||
| () -> { | ||
| d2.unsubscribe(s1); | ||
| d2.unsubscribe(s2); | ||
| connection.closeDispatcher(d1); | ||
| connection.closeDispatcher(d2); | ||
| }); | ||
| } |
There was a problem hiding this comment.
one way we use for cleanup is
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.
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.
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();
}| } | ||
|
|
||
| @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) | ||
| public static void onExit(@Advice.Enter Message message) {} |
There was a problem hiding this comment.
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.
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: