Fix requestEndTime is set after send response when empty content request#6587
Fix requestEndTime is set after send response when empty content request#6587gibeom-gwon wants to merge 1 commit intoline:mainfrom
Conversation
gibeom-gwon
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
📝 WalkthroughWalkthroughEmpty-content requests are marked ended during initialization: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (1)
44-54: Consider removing unusedverboseResponsesparameter.The
configureServermethod has an unusedverboseResponsesparameter. If it's not needed for future expansion, consider removing it to keep the code clean.♻️ Suggested simplification
@RegisterExtension static ServerExtension server = new ServerExtension() { @Override protected void configure(ServerBuilder sb) { - configureServer(sb, true); + sb.annotatedService(new AnnotatedService()); } }; - private static void configureServer(ServerBuilder sb, boolean verboseResponses) { - sb.annotatedService(new AnnotatedService()); - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.javacore/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.javacore/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🧬 Code graph analysis (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (2)
junit5/src/main/java/com/linecorp/armeria/testing/junit5/common/EventLoopExtension.java (1)
EventLoopExtension(45-97)junit5/src/main/java/com/linecorp/armeria/testing/junit5/server/ServerExtension.java (1)
ServerExtension(49-405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: Kubernetes Chaos test
- GitHub Check: flaky-tests
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: Summary
🔇 Additional comments (3)
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.java (1)
84-89: LGTM! The fix correctly addresses the timing issue.Calling
endRequest()duringinit()ensuresrequestEndTimeNanosis recorded immediately when the request context is initialized, since empty content requests have no additional data to read. This aligns with the documented behavior whererequestDurationNanosshould not include business logic processing and response transmission time.core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (2)
56-61: LGTM!The annotated service is minimal and appropriate for testing the empty content request scenario.
77-90: Good integration test for verifying the timing fix.The test correctly validates that
requestEndTimeNanosis recorded beforeresponseStartTimeNanosfor empty content requests, which is the core fix of this PR. The use ofrequestContextCaptor()to capture the server-side context is appropriate.
gibeom-gwon
force-pushed
the
fix-end-request-for-empty-content
branch
2 times, most recently
from
51d667a to
9b1cfc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (1)
44-54: Remove unusedverboseResponsesparameter.The
verboseResponsesparameter inconfigureServeris never used. Consider simplifying the configuration.Suggested simplification
@RegisterExtension static ServerExtension server = new ServerExtension() { @Override protected void configure(ServerBuilder sb) { - configureServer(sb, true); + sb.annotatedService(new AnnotatedService()); } }; - private static void configureServer(ServerBuilder sb, boolean verboseResponses) { - sb.annotatedService(new AnnotatedService()); - }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.javacore/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.javacore/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🧬 Code graph analysis (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (2)
junit5/src/main/java/com/linecorp/armeria/testing/junit5/common/EventLoopExtension.java (1)
EventLoopExtension(45-97)junit5/src/main/java/com/linecorp/armeria/testing/junit5/server/ServerExtension.java (1)
ServerExtension(49-405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: flaky-tests
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: site
- GitHub Check: lint
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (3)
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.java (1)
83-90: LGTM! Clean fix for the request timing issue.The change correctly ends the request immediately during initialization since
EmptyContentDecodedHttpRequesthas no body data to read. This ensuresrequestEndTimeNanosis recorded before business logic and response transmission, as documented.core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (2)
56-61: LGTM!Simple and appropriate test endpoint for validating empty-content request timing.
77-90: LGTM! Good integration test validating the timing fix.The test correctly verifies that
requestEndTimeNanosis recorded beforeresponseStartTimeNanosfor empty-content GET requests, which is the core behavior this PR fixes. Uses JUnit 5 and AssertJ as per coding guidelines.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (1)
73-86: Consider a more semantically accurate test name.The test logic correctly validates the fix. However, the method name uses "IsFasterThan" when comparing timestamps—times are "earlier" or "later," not "faster" or "slower." Consider renaming for clarity:
requestEndTimeNanosPrecedesResponseStartTimeNanosrequestEndTimeNanosIsLessThanResponseStartTimeNanosrequestEndsBeforeResponseStarts♻️ Optional: rename for semantic clarity
@Test - void requestEndTimeNanosIsFasterThanResponseStartTimeNanos() + void requestEndTimeNanosPrecedesResponseStartTimeNanos() throws InterruptedException {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.javacore/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequest.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: - The primary coding conventions and style guide for this project are defined insite/src/pages/community/developer-guide.mdx. Please strictly adhere to this file as the ultimate source of truth for all style and convention-related feedback.2. Specific check for
@UnstableApi
- Review all newly added public classes and methods to ensure they have the
@UnstableApiannotation.- However, this annotation is NOT required under the following conditions:
- If the class or method is located in a package containing
.internalor.testing.- If the class or method is located in a test source set.
- If a public method is part of a class that is already annotated with
@UnstableApi.
Files:
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java
🧬 Code graph analysis (1)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (2)
junit5/src/main/java/com/linecorp/armeria/testing/junit5/common/EventLoopExtension.java (1)
EventLoopExtension(45-97)junit5/src/main/java/com/linecorp/armeria/testing/junit5/server/ServerExtension.java (1)
ServerExtension(49-405)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: build-ubicloud-standard-16-jdk-25
- GitHub Check: build-macos-latest-jdk-25
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-11
- GitHub Check: build-ubicloud-standard-16-jdk-17-leak
- GitHub Check: build-ubicloud-standard-16-jdk-17-min-java-17-coverage
- GitHub Check: build-ubicloud-standard-16-jdk-11
- GitHub Check: build-ubicloud-standard-16-jdk-21-snapshot-blockhound
- GitHub Check: build-ubicloud-standard-16-jdk-8
- GitHub Check: build-windows-latest-jdk-25
- GitHub Check: flaky-tests
- GitHub Check: lint
- GitHub Check: site
- GitHub Check: Kubernetes Chaos test
- GitHub Check: Summary
🔇 Additional comments (3)
core/src/test/java/com/linecorp/armeria/server/EmptyContentDecodedHttpRequestTest.java (3)
25-35: LGTM!All new imports are necessary for the added test infrastructure and test method.
44-50: LGTM!The
ServerExtensionsetup follows the established pattern, correctly wiring the annotated service for integration testing.
52-57: LGTM!The
AnnotatedServiceis appropriately minimal for testing empty-content GET requests. Per coding guidelines,@UnstableApiis not required since this is in a test source set.
|
|
||
| // EmptyContentDecodedHttpRequest does not have any additional data to read. | ||
| // End the request immediately. | ||
| ctx.logBuilder().endRequest(); |
There was a problem hiding this comment.
There are some regressions due to this changes.
AnnotatedServiceContentTest > fileTypeReturn() FAILED
java.lang.AssertionError:
Expecting actual not to be null
at com.linecorp.armeria.internal.server.annotation.AnnotatedServiceContentTest.fileTypeReturn(AnnotatedServiceContentTest.java:219)
AnnotatedServiceRequestLogNameTest > customServiceNameWithDecorator() FAILED
org.opentest4j.AssertionFailedError:
expected: "DecoratedService"
but was: "MyBarService"
at app//com.linecorp.armeria.internal.server.annotation.AnnotatedServiceRequestLogNameTest.customServiceNameWithDecorator(AnnotatedServiceRequestLogNameTest.java:135)
What do you think of introducing RequestLogBuilder.requestEndTimeNanos(requestEndTimeNanos) to manually recode the end time?
| ctx.logBuilder().endRequest(); | |
| ctx.logBuilder().requestEndTimeNanos(System.nanoTime()); |
There was a problem hiding this comment.
I think that's a good way.
If REQUEST_END_TIME is already set, I think it will be resolved by not overwriting it in endRequest().
The one minor thing I'm concern is, if TimeNanos keyword is included in the method name, I think it will be inconsistent with existing similar implementation (requestFirstBytesTransferred()). However, if we remove it, I think this can be confused with endRequest(). How do you think about the method naming?
There was a problem hiding this comment.
Although requestEndTimeNanos() looks verbose, I prefer a clearer name. I'm open to better suggestions.
gibeom-gwon
force-pushed
the
fix-end-request-for-empty-content
branch
from
9b1cfc4 to
0f03dfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java`:
- Around line 192-200: The two newly added public methods
RequestLogBuilder.requestEnd() and RequestLogBuilder.requestEnd(long) must be
annotated with `@UnstableApi` per project guidelines; update the method
declarations to add the `@UnstableApi` annotation and ensure the corresponding
import for the annotation is added (import
com.linecorp.armeria.common.annotation.UnstableApi or the correct package used
in the project) so the public API is properly marked unstable.
core/src/main/java/com/linecorp/armeria/common/logging/RequestLogBuilder.java
Show resolved
Hide resolved
gibeom-gwon
force-pushed
the
fix-end-request-for-empty-content
branch
from
0f03dfe to
e14a5d3
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6587 +/- ##
============================================
- Coverage 74.46% 74.40% -0.07%
- Complexity 22234 23715 +1481
============================================
Files 1963 2128 +165
Lines 82437 88605 +6168
Branches 10764 11586 +822
============================================
+ Hits 61385 65923 +4538
- Misses 15918 17147 +1229
- Partials 5134 5535 +401 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Motivation:
requestEndTimeNanos is set after send response when processing the empty body request (ex. GET).
This led to the inclusion of business logic processing time and response transmission time in the
requestDurationNanos.Modifications:
Result:
requestDurationNanosreturns expected durations as documented.