-
Notifications
You must be signed in to change notification settings - Fork 921
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
Support micrometer context-propagation #5577
Conversation
dce5d4d
to
faffdd6
Compare
...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? |
Hi @ikhoon nim, orry to bother you. |
It has been a long time since the last review. |
Hi! @chickenchickenlove. Apologies for the late reply. The current implementation has a limitation—it doesn’t support the context hook due to the design of the Micrometer context propagation API. Specifically, the setValue() method doesn’t allow returning a Closeable instance. Because of this limitation, we cannot place these classes in the core module. Before we can address this issue, we need to discuss potential solutions with the Micrometer maintainers. In the meantime, we thought it would be a good idea to move these classes to a separate module, like Would you mind creating another module for this? |
@minwoox -nim, thanks for your comments 🙇♂️
|
Yeah, please go ahead. 😉 |
f220487
to
640f904
Compare
@minwoox -nim, Thanks for your comments! |
reactor3/build.gradle
Outdated
@@ -1,3 +1,5 @@ | |||
dependencies { | |||
api libs.reactor.core | |||
implementation libs.context.propagation | |||
implementation project(':micrometer-context') |
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 exclude this micrometer-context
module from reactor3?
micrometer-context
has a limitation which isn't supporting the context hook so we cannot include it in reactor3 module by default.
Here's the suggestion:
// reactor3/build.gradle
dependencies {
api libs.reactor.core
}
// micrometer-context/build.gradle
dependencies {
implementation libs.context.propagation
testImplementation project(':reactor3')
}
and move all test classes into micrometer-context
module.
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 , thanks for your comments!
I fixed it!
Please take another look. 🙇♂️
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.
Looks great. 👍
Thanks a lot for your hard work. 🙇♂️
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.
👍 👍
...n/java/com/linecorp/armeria/common/micrometer/context/RequestContextThreadLocalAccessor.java
Show resolved
Hide resolved
...meter-context/src/main/java/com/linecorp/armeria/common/micrometer/context/package-info.java
Show resolved
Hide resolved
…icrometer/context/package-info.java Co-authored-by: jrhee17 <[email protected]>
…icrometer/context/RequestContextThreadLocalAccessor.java Co-authored-by: jrhee17 <[email protected]>
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.
🙇♂️🙇♂️
It is okay to merge this PR when CI builds pass. |
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()
.