Spring webflux subscribe #190
Conversation
There was a problem hiding this comment.
Based on previous discussions and positive feedback on the changes I'm ok with these changes.
Out of curiosity, have any kind of performance tests been run to determine the impact of the additional token creation?
Also, before closing this out it would be great to determine whether or not it also addresses this issue: #125
| * * Copyright 2020 New Relic Corporation. All rights reserved. | ||
| * * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| */ |
There was a problem hiding this comment.
This header needs to be added to all source files.
There was a problem hiding this comment.
Does it? Some of this code was based off open telemetry instrumentation (as called out in comments and the NOTICE.txt in the root of these modules. Was the omission of these headers in the 0.8.0 module accidental or intentional?
There was a problem hiding this comment.
It was a bit of both, I'll add the headers to the classes that aren't derived from Otel
|
|
||
| import com.newrelic.api.agent.weaver.SkipIfPresent; | ||
|
|
||
| @SkipIfPresent |
There was a problem hiding this comment.
In reactor-netty versions prior to 0.9.0, LambdaSubscriber::currentContext is implemented in the super class, which we can't weave so we needed to split the instrumentation. This is to prevent netty-reactor-0.8 from applying to 0.9+ and effectively double weaving.
| // to be linked by TokenLinkingSubscriber but expired on onComplete here | ||
| if (AgentBridge.getAgent().getTransaction(false) != null | ||
| && initialContext.getOrDefault("newrelic-token", null) == null) { | ||
| nrContext = Context.of("newrelic-token", NewRelic.getAgent().getTransaction().getToken()); |
There was a problem hiding this comment.
I believe we discussed naming this newrelic-token field to something else to disambiguate it from the key of the other context.
There was a problem hiding this comment.
Doing so made the code considerably more complicated and brittle. This way we continue to use the TokenLinkingSubscriber rather a new Subscriber.
| } | ||
| return Weaver.callOriginal(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the expiring of tokens you said was being done defensively, but which not actually needed, correct?
There was a problem hiding this comment.
Yes, it turns out that the writeWith and writeAndFlushWith can get called during the reactive flow which prevents further operations from linking the token and effectively ending the Transaction early.
When calling subscribe on a Mono/Flux, a LambdaSubscriber is created with a new Context. This change inserts a newrelic-token on said Context for the TokenLinkingSubscriber.
I had to split the netty-reactor-0.8.0 instrumentation and create one for 0.9.0, because the earlier versions do not provide an impl of currentContext and we can't indirect weave inherited methods.
I also removed the writeWith and writeAndFlushWith weave methods from spring-webflux, as they were expiring the Token too early and allowing the Transaction to finish before the task was fully complete.