-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix of transaction propagation for empty mono/flux and retry operators #237
Fix of transaction propagation for empty mono/flux and retry operators #237
Conversation
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.
Thanks @veklov . This all looks good. Just two minor changes if you don't mind
if (NettyReactorConfig.errorsEnabled) { | ||
// I believe 100% of users disable this as it makes NewRelic to report caught and handled | ||
// exceptions as errors in APM. Is there a real use case for this? |
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.
Would you be OK with setting the default to false and leaving the config in case someone finds it useful
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 just wanted to draw your attention to this. No change is necessary if you think it may be useful for someone.
Please also note that there is a similar lifter in spring-webflux instrumentation module. It reports such error unconditionally and expires tokens immediately after linking (one of the issues being fixed in this PR).
Lines 74 to 92 in 2ff0a81
@Trace(async = true, excludeFromTransactionTrace = true) | |
private void withNRToken(Runnable runnable) { | |
Token token = currentContext().get(NR_TOKEN); | |
if (token != null) { | |
token.linkAndExpire(); | |
} | |
runnable.run(); | |
} | |
@Trace(async = true, excludeFromTransactionTrace = true) | |
private void withNRError(Runnable runnable, Throwable throwable) { | |
Token token = currentContext().get(NR_TOKEN); | |
if (token != null && token.isActive()) { | |
token.linkAndExpire(); | |
NewRelic.noticeError(throwable); | |
} | |
runnable.run(); | |
} | |
} |
I use spring webflux server and spring webclient with reactor-netty within the same application. Given that neither token expiration nor error reporting happens after I apply this PR and configure reactor-netty instrumentation to not report errors, it seems that the webflux's lifter is not used. This creates a lot of confusion for understanding the instrumentation.
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.
Thanks for the pointer, I'll revisit that instrumentation and see if we can't clean it up some
} | ||
|
||
@Test | ||
@Ignore("A test harness check") |
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.
Can you remove or un-ignore all of the ignored tests
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.
Done
This PR fixes two issues with the project reactor instrumentation