Conversation
PreAgile
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
|
|
📝 WalkthroughWalkthroughThis PR adds new BuiltInProperty enums exposing request/response start/end timestamps, durations (nanos/millis), total durations, content previews, serialization format, session protocol, and host; and updates tests to assert and validate these exports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6683 +/- ##
============================================
- Coverage 74.46% 74.02% -0.44%
- Complexity 22234 24113 +1879
============================================
Files 1963 2173 +210
Lines 82437 90442 +8005
Branches 10764 11858 +1094
============================================
+ Hits 61385 66950 +5565
- Misses 15918 17881 +1963
- Partials 5134 5611 +477 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| HOST("host", log -> { | ||
| if (log.isAvailable(RequestLogProperty.REQUEST_HEADERS)) { | ||
| final String authority = log.requestHeaders().authority(); | ||
| if (authority != null && !"?".equals(authority)) { |
There was a problem hiding this comment.
Question: I'm wondering why we need to compare ??
There was a problem hiding this comment.
Note) The changes in this file are mostly directly ported from AccessLogComponent.
I agree we can change the logic, but we may want to change both sides if we decide to update in this PR or leave for future works, I'm fine with either direction.
There was a problem hiding this comment.
If so, let's keep it as is and fix it separately if necessary.
There was a problem hiding this comment.
Understood. The ? check is technically unreachable here since isAvailable(REQUEST_HEADERS) guards against the dummy headers, but keeping it as is to stay consistent with AccessLogComponent — if we change it, both sides should be updated together.
| }), | ||
|
|
||
| /** | ||
| * {@code "host"} - the authority of the request, derived from the {@code :authority} header. |
There was a problem hiding this comment.
Note that log.requestHeaders().authority() falls back to Host header for HTTP/1.1
There was a problem hiding this comment.
Thanks for the note. I see that RequestHeaders.authority() internally falls back to the Host header for HTTP/1.1, so this property works regardless of the protocol version. Updated the Javadoc to mention the Host header fallback for HTTP/1.1.
| */ | ||
| RES_DURATION_MILLIS("res.duration_millis", log -> { | ||
| if (log.isAvailable(RequestLogProperty.RESPONSE_END_TIME)) { | ||
| return String.valueOf(Duration.ofNanos(log.responseDurationNanos()).toMillis()); |
There was a problem hiding this comment.
Duration.ofNanos() creates an instance just for this time conversion.
| return String.valueOf(Duration.ofNanos(log.responseDurationNanos()).toMillis()); | |
| return String.valueOf(TimeUnit.NANOSECONDS.toMillis(log.responseDurationNanos())); |
There was a problem hiding this comment.
Thanks for the suggestion. Replaced all Duration.ofNanos().toMillis() with TimeUnit.NANOSECONDS.toMillis() to avoid unnecessary heap allocation in the converter lambdas. Also applied the same principle to Instant usages in REQ_END_TIME_MILLIS and RES_END_TIME_MILLIS — replaced with plain millis arithmetic since the existing converters in this file (e.g. ELAPSED_NANOS) consistently use direct primitive-to-String conversion without intermediate objects.
|
Thank you for your interest in the Armeria repository and for contributing to it. It seems you are interested in other issues as well, but please understand that reviews consume a significant amount of our resources, so they may not be completed quickly. If your goal is to improve your understanding of Armeria core, I would recommend creating PRs without using AI. |
- LINE Pay Backend Engineer JD 분석 (필수/우대 요건) - Armeria 기여 ↔ JD 연결 테이블 (대량 트래픽, 분산시스템, 감사 로깅) - 면접 스토리 구체적 에피소드 정리 (PR line#6683, issue-6385) - 우선순위: 실제 서비스 경험 > CS 기본기 > Armeria 기여(보조) - README에 커리어 전략 문서 링크 추가 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace Duration.ofNanos().toMillis() with TimeUnit.NANOSECONDS.toMillis() for REQ_DURATION_MILLIS, RES_DURATION_MILLIS, and TOTAL_DURATION_MILLIS - Replace Instant-based end time calculation with plain millis arithmetic for REQ_END_TIME_MILLIS and RES_END_TIME_MILLIS - Update HOST Javadoc to note Host header fallback for HTTP/1.1
PreAgile
force-pushed
the
feature/add-builtin-properties
branch
from
0180453 to
92390d3
Compare
|
Thank you for the detailed review. I've pushed a new commit addressing each point — switching to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
logback/logback12/src/test/java/com/linecorp/armeria/common/logback/RequestContextExportingAppenderTest.java (1)
677-701: Optionally strengthen timing consistency assertions.Current checks validate equality/non-negativity; you can also assert millis values are derived from nanos to catch conversion regressions.
♻️ Optional tightening
void testTimingValueConsistency() throws Exception { @@ final ILoggingEvent e = log(events); final Map<String, String> mdc = e.getMDCPropertyMap(); + assertThat(mdc).containsKeys("elapsed_nanos", "total_duration_nanos", "total_duration_millis", + "req.duration_nanos", "req.duration_millis"); // total_duration_nanos and elapsed_nanos should have the same value. assertThat(mdc.get("total_duration_nanos")).isEqualTo(mdc.get("elapsed_nanos")); - // duration values should be non-negative. - assertThat(Long.parseLong(mdc.get("total_duration_nanos"))).isNotNegative(); - assertThat(Long.parseLong(mdc.get("total_duration_millis"))).isNotNegative(); - assertThat(Long.parseLong(mdc.get("req.duration_nanos"))).isNotNegative(); - assertThat(Long.parseLong(mdc.get("req.duration_millis"))).isNotNegative(); + final long totalDurationNanos = Long.parseLong(mdc.get("total_duration_nanos")); + final long totalDurationMillis = Long.parseLong(mdc.get("total_duration_millis")); + final long reqDurationNanos = Long.parseLong(mdc.get("req.duration_nanos")); + final long reqDurationMillis = Long.parseLong(mdc.get("req.duration_millis")); + + // duration values should be non-negative. + assertThat(totalDurationNanos).isNotNegative(); + assertThat(totalDurationMillis).isNotNegative(); + assertThat(reqDurationNanos).isNotNegative(); + assertThat(reqDurationMillis).isNotNegative(); + + // millis values should match nanos-to-millis conversion. + assertThat(totalDurationMillis) + .isEqualTo(java.util.concurrent.TimeUnit.NANOSECONDS.toMillis(totalDurationNanos)); + assertThat(reqDurationMillis) + .isEqualTo(java.util.concurrent.TimeUnit.NANOSECONDS.toMillis(reqDurationNanos)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logback/logback12/src/test/java/com/linecorp/armeria/common/logback/RequestContextExportingAppenderTest.java` around lines 677 - 701, The test testTimingValueConsistency currently asserts equality and non-negativity only; strengthen it by adding checks that the millisecond fields are consistent with their nanosecond counterparts (i.e., total_duration_millis == total_duration_nanos / 1_000_000 and req.duration_millis == req.duration_nanos / 1_000_000) using values pulled from the MDC produced by log(events); update assertions in testTimingValueConsistency (alongside prepare, newClientContext, and log usages) to parse nanos and millis as longs and assert the integer division conversion matches to catch conversion regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@logback/logback12/src/test/java/com/linecorp/armeria/common/logback/RequestContextExportingAppenderTest.java`:
- Around line 677-701: The test testTimingValueConsistency currently asserts
equality and non-negativity only; strengthen it by adding checks that the
millisecond fields are consistent with their nanosecond counterparts (i.e.,
total_duration_millis == total_duration_nanos / 1_000_000 and
req.duration_millis == req.duration_nanos / 1_000_000) using values pulled from
the MDC produced by log(events); update assertions in testTimingValueConsistency
(alongside prepare, newClientContext, and log usages) to parse nanos and millis
as longs and assert the integer division conversion matches to catch conversion
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f77df26b-020b-4427-9e58-520ef9c5d1fd
📒 Files selected for processing (3)
core/src/main/java/com/linecorp/armeria/common/logging/BuiltInProperty.javacore/src/test/java/com/linecorp/armeria/common/logging/RequestContextExporterTest.javalogback/logback12/src/test/java/com/linecorp/armeria/common/logback/RequestContextExportingAppenderTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/linecorp/armeria/common/logging/RequestContextExporterTest.java
- core/src/main/java/com/linecorp/armeria/common/logging/BuiltInProperty.java
5 participants
Motivation:
BuiltInPropertycurrently provides a limited set of properties compared toAccessLogComponent'sRequestLogComponent. Properties such as request/responsetiming, content preview, serialization format, session protocol, and host are
available in access log formatting but cannot be exported to MDC via
RequestContextExportingAppender.Modifications:
req.start_time_millis,req.end_time_millis,req.duration_nanos,req.duration_millis,res.start_time_millis,res.end_time_millis,res.duration_nanos,res.duration_millis,total_duration_nanos,total_duration_millisreq.content_preview,res.content_previewserialization.format,session.protocol,hostelapsed_nanosremains for backward compatibility;total_duration_*is addedfor alignment with
AccessLogComponentreq.content_preview/res.content_previewreturn only the content previewwithout falling back to RPC request parameters, unlike
req.content/res.contentResult: