-
Notifications
You must be signed in to change notification settings - Fork 905
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
Open
chickenchickenlove
wants to merge
43
commits into
line:main
Choose a base branch
from
chickenchickenlove:240408-context-propagation
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
faffdd6
Support micrometer context-propagation : Skeleton code
chickenchickenlove 0e1fd49
Merge branch 'main' into 240408-context-propagation
chickenchickenlove 48b4039
Resolve lint error
chickenchickenlove 65aa41b
Add java docs
chickenchickenlove 1c3cda0
Add java docs and modify unit test
chickenchickenlove 8030e8f
Apply comment
chickenchickenlove 46cd890
remove ContextPropagation from boot3-autoconfigure
chickenchickenlove 67a4f14
Move RequestContextAccessor to core module.
chickenchickenlove ae022ad
remove useless dependency
chickenchickenlove c633eaa
add dependencies
chickenchickenlove 8710f1d
resolve java doc error
chickenchickenlove 64cb196
fix test fail
chickenchickenlove 9b32927
remove unused imported
chickenchickenlove 6d4762a
Add test cases
chickenchickenlove 4209756
Add test case
chickenchickenlove 747fa36
Update core/src/main/java/com/linecorp/armeria/common/RequestContextA…
chickenchickenlove 83a84b0
Update core/src/main/java/com/linecorp/armeria/common/RequestContextA…
chickenchickenlove 3bdbd3e
Update core/src/main/java/com/linecorp/armeria/common/RequestContextA…
chickenchickenlove 767bd9a
Fix lint error and test case
chickenchickenlove 88b1475
resolve lint error
chickenchickenlove 4a3ebfc
remove RequestContextPropagationHooks.
chickenchickenlove 6a3d4f4
remove init context for Stepverifier.
chickenchickenlove 7d2970d
Add test cases for contextCapture().
chickenchickenlove 24c6f38
Should not ctx in main thread.
chickenchickenlove 4a00814
apply review
chickenchickenlove f23f670
add @UnstableAPI and remove whitespace.
chickenchickenlove eb0c71e
Update core/src/test/java/com/linecorp/armeria/common/RequestContextA…
chickenchickenlove 8a46ca5
apply review
chickenchickenlove aca31f8
Merge branch 'main' into 240408-context-propagation
chickenchickenlove 9cd60e1
apply review
chickenchickenlove 4b5015f
add test case
chickenchickenlove 90f76a7
fix lint error
chickenchickenlove 230a664
Update core/src/main/java/com/linecorp/armeria/internal/common/Reques…
chickenchickenlove 3b9de29
Update core/src/test/java/com/linecorp/armeria/internal/common/Reques…
chickenchickenlove 890a725
apply review
chickenchickenlove 6a3e46e
Update core/src/main/java/com/linecorp/armeria/internal/common/Reques…
chickenchickenlove e80117c
apply review
chickenchickenlove 12a2929
Merge branch 'main' into 240408-context-propagation
trustin 2ca6b1b
Merge branch 'main' into 240408-context-propagation
jrhee17 79f16af
Update reactor3/src/test/java/com/linecorp/armeria/common/reactor3/Re…
chickenchickenlove fb2db7d
Update reactor3/src/test/java/com/linecorp/armeria/common/reactor3/Re…
chickenchickenlove f087912
Update core/src/main/java/com/linecorp/armeria/internal/common/Reques…
chickenchickenlove 1e6cd9b
Add validation callback for context-propagation.
chickenchickenlove File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
101 changes: 101 additions & 0 deletions
101
...src/main/java/com/linecorp/armeria/internal/common/RequestContextThreadLocalAccessor.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
/* | ||
* Copyright 2024 LINE Corporation | ||
* | ||
* LINE Corporation licenses this file to you under the Apache License, | ||
* version 2.0 (the "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at: | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
* License for the specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package com.linecorp.armeria.internal.common; | ||
|
||
import org.reactivestreams.Subscription; | ||
|
||
import com.linecorp.armeria.common.RequestContext; | ||
import com.linecorp.armeria.common.RequestContextStorage; | ||
|
||
import io.micrometer.context.ContextRegistry; | ||
import io.micrometer.context.ContextSnapshot; | ||
import io.micrometer.context.ContextSnapshot.Scope; | ||
import io.micrometer.context.ThreadLocalAccessor; | ||
|
||
/** | ||
* This class works with the | ||
* <a href="https://docs.micrometer.io/context-propagation/reference/index.html">Micrometer | ||
* Context Propagation</a> to keep the {@link RequestContext} during | ||
* <a href="https://github.com/reactor/reactor-core">Reactor</a> operations. | ||
* Get the {@link RequestContextThreadLocalAccessor} to register it to the {@link ContextRegistry}. | ||
* Then, {@link ContextRegistry} will use {@link RequestContextThreadLocalAccessor} to | ||
* propagate context during the | ||
* <a href="https://github.com/reactor/reactor-core">Reactor</a> operations | ||
* so that you can get the context using {@link RequestContext#current()}. | ||
* However, please note that you should include Mono#contextWrite(ContextView) or | ||
* Flux#contextWrite(ContextView) to end of the Reactor codes. | ||
* If not, {@link RequestContext} will not be keep during Reactor Operation. | ||
*/ | ||
public final class RequestContextThreadLocalAccessor implements ThreadLocalAccessor<RequestContext> { | ||
|
||
private static final Object KEY = RequestContext.class; | ||
|
||
/** | ||
* The value which obtained through {@link RequestContextThreadLocalAccessor}, | ||
* will be stored in the Context under this {@code KEY}. | ||
* This method will be called by {@link ContextSnapshot} internally. | ||
*/ | ||
@Override | ||
public Object key() { | ||
return KEY; | ||
} | ||
|
||
/** | ||
* {@link ContextSnapshot} will call this method during the execution | ||
* of lambda functions in {@link ContextSnapshot#wrap(Runnable)}, | ||
* as well as during Mono#subscribe(), Flux#subscribe(), | ||
* {@link Subscription#request(long)}, and CoreSubscriber#onSubscribe(Subscription). | ||
* Following these calls, {@link ContextSnapshot#setThreadLocals()} is | ||
* invoked to restore the state of {@link RequestContextStorage}. | ||
* Furthermore, at the end of these methods, {@link Scope#close()} is executed | ||
* to revert the {@link RequestContextStorage} to its original state. | ||
*/ | ||
@Override | ||
public RequestContext getValue() { | ||
return RequestContext.currentOrNull(); | ||
} | ||
|
||
/** | ||
* {@link ContextSnapshot} will call this method during the execution | ||
* of lambda functions in {@link ContextSnapshot#wrap(Runnable)}, | ||
* as well as during Mono#subscribe(), Flux#subscribe(), | ||
* {@link Subscription#request(long)}, and CoreSubscriber#onSubscribe(Subscription). | ||
* Following these calls, {@link ContextSnapshot#setThreadLocals()} is | ||
* invoked to restore the state of {@link RequestContextStorage}. | ||
* Furthermore, at the end of these methods, {@link Scope#close()} is executed | ||
* to revert the {@link RequestContextStorage} to its original state. | ||
*/ | ||
@Override | ||
@SuppressWarnings("MustBeClosedChecker") | ||
public void setValue(RequestContext value) { | ||
RequestContextUtil.getAndSet(value); | ||
} | ||
|
||
/** | ||
* This method will be called at the start of {@link ContextSnapshot.Scope} and | ||
* the end of {@link ContextSnapshot.Scope}. If reactor Context does not | ||
* contains {@link RequestContextThreadLocalAccessor#KEY}, {@link ContextSnapshot} will use | ||
* this method to remove the value from {@link ThreadLocal}. | ||
* Please note that {@link RequestContextUtil#pop()} return {@link AutoCloseable} instance, | ||
* but it is not used in `Try with Resources` syntax. this is because {@link ContextSnapshot.Scope} | ||
* will handle the {@link AutoCloseable} instance returned by {@link RequestContextUtil#pop()}. | ||
*/ | ||
@Override | ||
@SuppressWarnings("MustBeClosedChecker") | ||
public void setValue() { | ||
RequestContextUtil.pop(); | ||
} | ||
} |
1 change: 1 addition & 0 deletions
1
core/src/main/resources/META-INF/services/io.micrometer.context.ThreadLocalAccessor
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
com.linecorp.armeria.internal.common.RequestContextThreadLocalAccessor |
168 changes: 168 additions & 0 deletions
168
...test/java/com/linecorp/armeria/internal/common/RequestContextThreadLocalAccessorTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
/* | ||
* Copyright 2024 LINE Corporation | ||
* | ||
* LINE Corporation licenses this file to you under the Apache License, | ||
* version 2.0 (the "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at: | ||
* | ||
* https://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
* License for the specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package com.linecorp.armeria.internal.common; | ||
|
||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
import java.util.List; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
import com.linecorp.armeria.client.ClientRequestContext; | ||
import com.linecorp.armeria.common.HttpMethod; | ||
import com.linecorp.armeria.common.HttpRequest; | ||
import com.linecorp.armeria.common.RequestContext; | ||
|
||
import io.micrometer.context.ContextRegistry; | ||
import io.micrometer.context.ContextSnapshot; | ||
import io.micrometer.context.ContextSnapshot.Scope; | ||
import io.micrometer.context.ContextSnapshotFactory; | ||
import io.micrometer.context.ThreadLocalAccessor; | ||
|
||
class RequestContextThreadLocalAccessorTest { | ||
|
||
@Test | ||
void should_be_loaded_by_SPI() { | ||
final ContextRegistry ctxRegistry = ContextRegistry.getInstance(); | ||
final List<ThreadLocalAccessor<?>> threadLocalAccessors = ctxRegistry.getThreadLocalAccessors(); | ||
|
||
assertThat(threadLocalAccessors.size()).isGreaterThan(1); | ||
assertThat(threadLocalAccessors).hasAtLeastOneElementOfType(RequestContextThreadLocalAccessor.class); | ||
} | ||
|
||
@Test | ||
void should_return_expected_key() { | ||
// Given | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
final Object expectedValue = RequestContext.class; | ||
|
||
// When | ||
final Object result = reqCtxAccessor.key(); | ||
|
||
// Then | ||
assertThat(result).isEqualTo(expectedValue); | ||
} | ||
|
||
@Test | ||
@SuppressWarnings("MustBeClosedChecker") | ||
void should_success_set() { | ||
// Given | ||
final ClientRequestContext ctx = newContext(); | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
|
||
// When | ||
reqCtxAccessor.setValue(ctx); | ||
|
||
// Then | ||
final RequestContext currentCtx = RequestContext.current(); | ||
assertThat(currentCtx).isEqualTo(ctx); | ||
|
||
RequestContextUtil.pop(); | ||
} | ||
|
||
@Test | ||
void should_throw_NPE_when_set_null() { | ||
// Given | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
|
||
// When + Then | ||
assertThatThrownBy(() -> reqCtxAccessor.setValue(null)) | ||
.isInstanceOf(NullPointerException.class); | ||
} | ||
|
||
@Test | ||
void should_be_null_when_setValue() { | ||
// Given | ||
final ClientRequestContext ctx = newContext(); | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
reqCtxAccessor.setValue(ctx); | ||
|
||
// When | ||
reqCtxAccessor.setValue(); | ||
|
||
// Then | ||
final RequestContext reqCtx = RequestContext.currentOrNull(); | ||
assertThat(reqCtx).isNull(); | ||
} | ||
|
||
@Test | ||
@SuppressWarnings("MustBeClosedChecker") | ||
void should_be_restore_original_state_when_restore() { | ||
// Given | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
final ClientRequestContext previousCtx = newContext(); | ||
final ClientRequestContext currentCtx = newContext(); | ||
reqCtxAccessor.setValue(currentCtx); | ||
|
||
// When | ||
reqCtxAccessor.restore(previousCtx); | ||
|
||
// Then | ||
final RequestContext reqCtx = RequestContext.currentOrNull(); | ||
assertThat(reqCtx).isNotNull(); | ||
assertThat(reqCtx).isEqualTo(previousCtx); | ||
|
||
RequestContextUtil.pop(); | ||
} | ||
|
||
@Test | ||
void should_be_null_when_restore() { | ||
// Given | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
final ClientRequestContext currentCtx = newContext(); | ||
reqCtxAccessor.setValue(currentCtx); | ||
|
||
// When | ||
reqCtxAccessor.restore(); | ||
|
||
// Then | ||
final RequestContext reqCtx = RequestContext.currentOrNull(); | ||
assertThat(reqCtx).isNull(); | ||
} | ||
|
||
@Test | ||
void requestContext_should_exist_inside_scope_and_not_outside() { | ||
// Given | ||
final RequestContextThreadLocalAccessor reqCtxAccessor = new RequestContextThreadLocalAccessor(); | ||
final ClientRequestContext currentCtx = newContext(); | ||
final ClientRequestContext expectedCtx = currentCtx; | ||
reqCtxAccessor.setValue(currentCtx); | ||
|
||
final ContextSnapshotFactory factory = ContextSnapshotFactory.builder() | ||
.clearMissing(true) | ||
.build(); | ||
final ContextSnapshot contextSnapshot = factory.captureAll(); | ||
reqCtxAccessor.setValue(); | ||
|
||
// When : contextSnapshot.setThreadLocals() | ||
try (Scope ignored = contextSnapshot.setThreadLocals()) { | ||
|
||
// Then : should not | ||
final RequestContext reqCtxInScope = RequestContext.currentOrNull(); | ||
assertThat(reqCtxInScope).isNotNull(); | ||
assertThat(reqCtxInScope).isSameAs(expectedCtx); | ||
} | ||
|
||
// Then | ||
final RequestContext reqCtxOutOfScope = RequestContext.currentOrNull(); | ||
assertThat(reqCtxOutOfScope).isNull(); | ||
} | ||
|
||
static ClientRequestContext newContext() { | ||
return ClientRequestContext.of(HttpRequest.of(HttpMethod.GET, "/")); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
dependencies { | ||
api libs.reactor.core | ||
implementation libs.context.propagation | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm fine with the current implementation since it is analogous to the current
RequestContextHooks
.Going forward, I'm thinking:
RequestContext
RequestContext#[push|pop]
APIs - we probably don't want to make a breaking change on this behaviorRequestContextThreadLocalAccessor
positioned between theRequestContext
andRequestContextStorage
implementation is more realistic.e.g.
This way, we could 1) keep compatibility with the current
RequestContext
API 2) Allow an extension point for other context objects to be passed around 3) integration with thecontext-propagation
API can be more first-class.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.
Good point. I'm down.
Also, we can introduce
Context
interface to make new scope context extension easy for the future.(For example,
RequestContext
is perRequest
,ConnectionContext
is perConnection
if armeria supports that features in the future. )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.
Clarify a little bit: We don't want to use Micrometer API to push or pop the context in our core. What we're trying to achieve with the integration is to allow users to access
RequestContext
(or the context types from other frameworks like Reactor) using Micrometer API.I agree that we will eventually want per-connection context, which is gonna be quite 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 see, I thought the point of integrating micrometer's context-propagation was to introduce ways for users to propagate thread local contexts other than
RequestContext
when using Armeria server/client.I'm fine with just letting users access
RequestContext
via the Micrometer API then.