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
Support micrometer context-propagation #5577
base: main
Are you sure you want to change the base?
Support micrometer context-propagation #5577
Conversation
dce5d4d
to
faffdd6
Compare
🔍 Build Scan® (commit: e80117c)
|
...utoconfigure/src/test/java/com/linecorp/armeria/spring/ArmeriaSettingsConfigurationTest.java
Outdated
Show resolved
Hide resolved
reactor3/src/main/java/com/linecorp/armeria/common/reactor3/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
reactor3/src/main/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationHook.java
Outdated
Show resolved
Hide resolved
reactor3/src/main/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationHook.java
Outdated
Show resolved
Hide resolved
reactor3/src/main/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationHook.java
Outdated
Show resolved
Hide resolved
I investigate that internal of Case 1.
|
@trustin nim, There are also major differences in the test. The test utility function private static <T> Mono<T> addCallbacks(Mono<T> mono, ClientRequestContext ctx) {
return mono.doFirst(() -> assertThat(ctxExists(ctx)).isTrue())
.doOnSubscribe(s -> assertThat(ctxExists(ctx)).isTrue())
.doOnRequest(l -> assertThat(ctxExists(ctx)).isTrue())
.doOnNext(foo -> assertThat(ctxExists(ctx)).isTrue())
.doOnSuccess(t -> assertThat(ctxExists(ctx)).isTrue())
.doOnEach(s -> assertThat(ctxExists(ctx)).isTrue())
.doOnError(t -> assertThat(ctxExists(ctx)).isTrue())
.doAfterTerminate(() -> assertThat(ctxExists(ctx)).isTrue())
// I added contextWrite(...)
.contextWrite(Context.of(RequestContextAccessor.getInstance().key(), ctx));
// doOnCancel and doFinally do not have context because we cannot add a hook to the cancel.
}
// Before : StepVerifier.create(mono1)
// After : Add initiali Reactor Context to StepVerifier.
StepVerifier.create(mono1, initialReactorContext(ctx))
.expectSubscriptionMatches(s -> ctxExists(ctx))
.expectNextMatches(s -> ctxExists(ctx) && "baz".equals(s))
.verifyComplete(); In previous test code,
Thus, initial Reactor Context should be include to |
Could you fix the build failures before getting reviews? |
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationFluxTest.java
Outdated
Show resolved
Hide resolved
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationFluxTest.java
Outdated
Show resolved
Hide resolved
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationFluxTest.java
Outdated
Show resolved
Hide resolved
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationFluxTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/RequestContextAccessorTest.java
Outdated
Show resolved
Hide resolved
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.
These look great! Thanks a lot for completing this PR. 👍 👍 👍
core/src/main/java/com/linecorp/armeria/common/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/RequestContextAccessorTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/RequestContextAccessorTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/RequestContextAccessorTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/com/linecorp/armeria/common/RequestContextAccessorTest.java
Outdated
Show resolved
Hide resolved
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationMonoTest.java
Outdated
Show resolved
Hide resolved
...r3/src/test/java/com/linecorp/armeria/common/reactor3/RequestContextPropagationFluxTest.java
Outdated
Show resolved
Hide resolved
…ccessorTest.java Co-authored-by: Trustin Lee <t@motd.kr>
core/src/main/java/com/linecorp/armeria/common/RequestContextAccessor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/common/RequestContextThreadLocalAccessor.java
Outdated
Show resolved
Hide resolved
...rc/test/java/com/linecorp/armeria/internal/common/RequestContextThreadLocalAccessorTest.java
Outdated
Show resolved
Hide resolved
…tContextThreadLocalAccessor.java Co-authored-by: Trustin Lee <t@motd.kr>
…tContextThreadLocalAccessorTest.java Co-authored-by: Trustin Lee <t@motd.kr>
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.
@minwoox IIRC you had some comments related with request context hooks you wanted @chickenchickenlove to address. Could you leave some comment about that?
core/src/main/java/com/linecorp/armeria/internal/common/RequestContextThreadLocalAccessor.java
Outdated
Show resolved
Hide resolved
…tContextThreadLocalAccessor.java Co-authored-by: Trustin Lee <t@motd.kr>
@Override | ||
@SuppressWarnings("MustBeClosedChecker") | ||
public void setValue(RequestContext value) { | ||
value.push(); |
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.
Let's just use RequestContextUtil.getAndSet(value);
.
value.push()
internally invokes a hook but the closeable
never be closed because we discard the result of value.push()
.
armeria/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java
Line 192 in 87f2318
final SafeCloseable closeable = invokeHook(current); |
armeria/core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java
Line 197 in 87f2318
closeable.close(); |
This is a limitation of the current design of Mircometer context-proparation.
Because we cannot call the closeable
, we should avoid invoking the hook altogether and inform users of the limitation of this API.
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.
@minwoox nim, thanks for your comments.
RequestContextUtil.getAndSet(value)
seems not get SafeClosable
instance.
I make new commit to apply your comments.
Please take another look, when you have free time 🙇♂️
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.
Still looks good. Thanks!
@Override | ||
@SuppressWarnings("MustBeClosedChecker") | ||
public void restore(RequestContext previousValue) { | ||
previousValue.push(); |
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.
ditto let's just use RequestContextUtil.getAndSet(previousValue);
Thanks! I left my opinion here: #5577 (comment) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5577 +/- ##
============================================
+ Coverage 73.95% 74.05% +0.09%
- Complexity 20115 21242 +1127
============================================
Files 1730 1850 +120
Lines 74161 78540 +4379
Branches 9465 10020 +555
============================================
+ Hits 54847 58164 +3317
- Misses 14837 15680 +843
- Partials 4477 4696 +219 ☔ View full report in Codecov by Sentry. |
Motivation:
Armeria
already support context-propagation to maintainRequestContext
during executing Reactor code. How it requires maintenance.Reactor
integratemicro-meter:context-propagation
to do context-propagation duringFlux
,Mono
officially. thus, it would be better to migrate fromRequestContextHook
toRequestContextPropagationHooks
because it can reduce maintenance cost.Modifications:
Hook
forReactor
.ThreadLocalAccessor
formicro-meter:context-propagation
to mainRequestContext
during executing Reactor code likeMono
,Flux
.enableContextPropagation
to integratemicro-meter:context-propagation
withspring-boot3
.Result:
micrometer:context-propagation
to maintainRequestContext
during executing Reactor code likeMono
,Flux
, just callRequestContextPropagationHook.enable()
.