Make RequestContextExtension.hook() public API #5107
Merged
Conversation
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
my4-dev
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
ikhoon
reviewed
Aug 10, 2023
core/src/main/java/com/linecorp/armeria/client/ClientOptions.java
Outdated
Show resolved
Hide resolved
| @@ -373,6 +375,9 @@ private void handleRequest(ChannelHandlerContext ctx, DecodedHttpRequest req) th | |||
| req, sslSession, proxiedAddresses, clientAddress, remoteAddress, localAddress, | |||
| req.requestStartTimeNanos(), req.requestStartTimeMicros()); | |||
|
|
|||
| reqCtx.hook(serviceCfg.contextHook()); | |||
| reqCtx.hook(config.contextHook()); | |||
There was a problem hiding this comment.
Should we move these to the constructor of DefaultServiceRequestContext
| channel, req, proxiedAddresses, clientAddress, remoteAddress, localAddress, routingCtx); | ||
|
|
||
| reqCtx.hook(config.contextHook()); |
There was a problem hiding this comment.
We can remove this line because the request is not exposed to users and no context is pushed.
| @@ -260,6 +262,17 @@ public AbstractClientOptionsBuilder endpointRemapper( | |||
| return option(ClientOptions.ENDPOINT_REMAPPER, endpointRemapper); | |||
| } | |||
|
|
|||
| /** | |||
| * Sets the {@link AutoCloseable} which will be called whenever this {@link RequestContext} is popped | |||
There was a problem hiding this comment.
This does not set an AutoCloseable but set a Supplier which produces an AutoCloseable. Should we update the Javadoc to reflect the actual behavior correctly?
| @Override | ||
| public ServiceConfigSetters contextHook(Supplier<? extends AutoCloseable> contextHook) { | ||
| requireNonNull(contextHook, "contextHook"); | ||
| this.contextHook = contextHook; |
There was a problem hiding this comment.
Should we make this API take multiple contextHook like NonWrappingRequestContext.hook()?
| @@ -348,7 +358,8 @@ ServiceConfig build(ServiceNaming defaultServiceNaming, | |||
| multipartUploadsLocation != null ? multipartUploadsLocation : defaultMultipartUploadsLocation, | |||
| ImmutableList.copyOf(shutdownSupports), | |||
| mergeDefaultHeaders(virtualHostDefaultHeaders.toBuilder(), defaultHeaders.build()), | |||
| requestIdGenerator != null ? requestIdGenerator : defaultRequestIdGenerator, errorHandler); | |||
| requestIdGenerator != null ? requestIdGenerator : defaultRequestIdGenerator, errorHandler, | |||
| contextHook != null ? contextHook : () -> (AutoCloseable) () -> {}); | |||
There was a problem hiding this comment.
Could we create singleton NOOP_CONTEXT_HOOK so that RequestContext.hook(Supplier) ignore that?
class {
public void hook(Supplier<? extends AutoCloseable> contextHook) {
if (contextHook == NOOP_CONTEXT_HOOK) {
return;
}
...
}
}
|
@ikhoon : Thanks for the advice! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5107 +/- ##
============================================
- Coverage 74.07% 74.03% -0.05%
- Complexity 20012 20013 +1
============================================
Files 1718 1718
Lines 73699 73753 +54
Branches 9383 9387 +4
============================================
+ Hits 54596 54604 +8
- Misses 14662 14704 +42
- Partials 4441 4445 +4
☔ View full report in Codecov by Sentry. |
minwoox
reviewed
Oct 13, 2023
core/src/main/java/com/linecorp/armeria/client/ClientOptions.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/client/AbstractClientOptionsBuilder.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContext.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/ContextPathAnnotatedServiceConfigSetters.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/HttpServerHandler.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/internal/server/DefaultServiceRequestContext.java
Outdated
Show resolved
Hide resolved
Co-authored-by: minux <[email protected]>
|
We are about to release the next version. Let me address other folks' comments to add this feature to the next release. |
minwoox
reviewed
Oct 18, 2023
core/src/main/java/com/linecorp/armeria/internal/common/RequestContextUtil.java
Outdated
Show resolved
Hide resolved
minwoox
reviewed
Oct 19, 2023
core/src/main/java/com/linecorp/armeria/client/AbstractClientOptionsBuilder.java
Show resolved
Hide resolved
trustin
approved these changes
Oct 20, 2023
core/src/main/java/com/linecorp/armeria/common/RequestContext.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContext.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/RequestContext.java
Outdated
Show resolved
Hide resolved
| @@ -85,7 +87,7 @@ public static void init() { /* no-op */ } | |||
| /** | |||
| * Returns the {@link SafeCloseable} which doesn't do anything. | |||
| */ | |||
| @MustBeClosed | |||
| @SuppressWarnings("MustBeClosedChecker") | |||
Comment on lines
+48
to
+53
| assertThat(overriddenMethods) | ||
| .as("%s is not overridden by %s", method, clazz) | ||
| .filteredOn(tMethod -> { | ||
| return method.getName().equals(tMethod.getName()) && | ||
| Arrays.equals(method.getParameterTypes(), tMethod.getParameterTypes()); | ||
| }).hasSize(1); |
ikhoon
approved these changes
Oct 20, 2023
core/src/main/java/com/linecorp/armeria/client/AbstractClientOptionsBuilder.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/server/ServiceConfig.java
Outdated
Show resolved
Hide resolved
jrhee17
reviewed
Oct 23, 2023
| @@ -172,6 +172,11 @@ public WebClientBuilder endpointRemapper( | |||
| return (WebClientBuilder) super.endpointRemapper(endpointRemapper); | |||
| } | |||
|
|
|||
| @Override | |||
| public WebClientBuilder contextHook(Supplier<? extends AutoCloseable> contextHook) { | |||
There was a problem hiding this comment.
We probably need to override the rest of the methods
e.g.
RestClient.builder()
.contextHook(() -> {})
.build();
minwoox
pushed a commit
that referenced
this pull request
Oct 25, 2023
Motivation: I found some overridden methods were missed. #5107 (comment) While I was at it, I also fixed a test which goes through the entire classpath looking for builders ending with the String `Builder`. Modifications: - Fixed `OverriddenBuilderMethodsReturnTypeTest` to not ignore missing methods - Fixed not overridden methods Result: - A more consistent fluent API experience <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
Bue-von-hon
pushed a commit
to Bue-von-hon/armeria
that referenced
this pull request
Nov 10, 2023
Motivation: Please refer to line#4991 Modifications: - Third-party libraries or users could use hook method to implement custom instrumentation for Armera servers or clients. Result: - Closes line#4991 - Third-party libraries or users could use contextHook method to add hook which is invoked whenever a `RequestContext` is popped from the `RequestContextStorage` like the following. ```java WebClient .builder() // `contextHook` should be attached when a new ClientRequestContext is created. .contextHook(contextHook) ... .build(); Server .builder() // `contextHook` should be attached when a new ServiceRequestContext is created. .contextHook(contextHook) ... .route().path("/api/...") .contextHook(serviceContextHook) .build(service) .build(); ``` <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
Bue-von-hon
pushed a commit
to Bue-von-hon/armeria
that referenced
this pull request
Nov 10, 2023
Motivation: I found some overridden methods were missed. line#5107 (comment) While I was at it, I also fixed a test which goes through the entire classpath looking for builders ending with the String `Builder`. Modifications: - Fixed `OverriddenBuilderMethodsReturnTypeTest` to not ignore missing methods - Fixed not overridden methods Result: - A more consistent fluent API experience <!-- Visit this URL to learn more about how to write a pull request description: https://armeria.dev/community/developer-guide#how-to-write-pull-request-description -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation:
Please refer to #4991
Modifications:
Result:
RequestContextExtension.hook()public API #4991RequestContextis popped from theRequestContextStoragelike the following.