Support micrometer context-propagation#5577
Conversation
dce5d4d to
faffdd6
Compare
|
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? |
|
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! |
| dependencies { | ||
| api libs.reactor.core | ||
| implementation libs.context.propagation | ||
| implementation project(':micrometer-context') |
There was a problem hiding this comment.
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.
@minwoox , thanks for your comments!
I fixed it!
Please take another look. 🙇♂️
minwoox
left a comment
There was a problem hiding this comment.
Looks great. 👍
Thanks a lot for your hard work. 🙇♂️
…icrometer/context/package-info.java Co-authored-by: jrhee17 <[email protected]>
…icrometer/context/RequestContextThreadLocalAccessor.java Co-authored-by: jrhee17 <[email protected]>
|
It is okay to merge this PR when CI builds pass. |






Motivation:
Armeriaalready support context-propagation to maintainRequestContextduring executing Reactor code. How it requires maintenance.Reactorintegratemicro-meter:context-propagationto do context-propagation duringFlux,Monoofficially. thus, it would be better to migrate fromRequestContextHooktoRequestContextPropagationHooksbecause it can reduce maintenance cost.Modifications:
HookforReactor.ThreadLocalAccessorformicro-meter:context-propagationto mainRequestContextduring executing Reactor code likeMono,Flux.enableContextPropagationto integratemicro-meter:context-propagationwithspring-boot3.Result:
micrometer:context-propagationto maintainRequestContextduring executing Reactor code likeMono,Flux, just callRequestContextPropagationHook.enable().