Skip to content

Extract grpc-status from responseContent when not in headers/trailers#6621

Open
sallylyen wants to merge 3 commits intoline:mainfrom
sallylyen:grpc-status-from-response-content
Open

Extract grpc-status from responseContent when not in headers/trailers#6621
sallylyen wants to merge 3 commits intoline:mainfrom
sallylyen:grpc-status-from-response-content

Conversation

@sallylyen
Copy link

Motivation

When a client cancels a gRPC request (e.g., via RST_STREAM), the grpc-status may not be present in response headers or trailers, causing metrics to be recorded with UNKNOWN status.

Resolves #6606

Modifications

  • Added fallback in GrpcMeterIdPrefixFunction to extract grpc-status from responseContent using Status.fromThrowable()
  • Added integration test for client cancellation scenario

Result

Motivation:
When a client cancels a gRPC request (e.g., via RST_STREAM), the
grpc-status may not be present in response headers or trailers,
causing metrics to be recorded with UNKNOWN status.

Resolves line#6606

Modifications:
- Added fallback in GrpcMeterIdPrefixFunction to extract grpc-status
  from responseContent using Status.fromThrowable()
- Added integration test for client cancellation scenario

Result:
- gRPC metrics now correctly record the status when client cancels

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@CLAassistant
Copy link

CLAassistant commented Feb 3, 2026

CLA assistant check
All committers have signed the CLA.

@coderabbitai
Copy link

coderabbitai bot commented Feb 3, 2026

📝 Walkthrough

Walkthrough

Added a fallback in GrpcMeterIdPrefixFunction that inspects RequestLog.responseContent() for an RpcResponse and, when headers/trailers lack grpc-status, derives and sets the grpc.status from the RPC response cause (e.g., client cancellation). Tests added to exercise the cancellation path.

Changes

Cohort / File(s) Summary
Fallback Status Extraction
grpc/src/main/java/.../GrpcMeterIdPrefixFunction.java
Added logic to check RequestLog.responseContent() for an RpcResponse when grpc-status is not present in headers/trailers; if the RPC completed exceptionally, derive a gRPC status from the cause, set the grpc.status tag, and return early.
Test Coverage & Helpers
grpc/src/test/java/.../GrpcMeterIdPrefixFunctionTest.java
Added grpcStatusFromResponseContent_clientCancellation test that simulates client cancellation via a low-level ClientCall and asserts grpc.status=CANCELLED; added helper to find client meters with configurable http.status and a delayed server method to enable cancellation testing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through logs where trailers hid,
I peered at response content when headers slid.
A cancelled call, a cause made plain,
I tag the status, trace the strain.
Metrics smile — no longer in vain. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: extracting grpc-status from responseContent when not in headers/trailers.
Description check ✅ Passed The description accurately explains the motivation, modifications, and expected outcome of the pull request.
Linked Issues check ✅ Passed The pull request directly addresses issue #6606 by implementing the fallback mechanism to extract grpc-status from responseContent and adding an integration test.
Out of Scope Changes check ✅ Passed All changes are directly scoped to resolving issue #6606: the fallback logic in GrpcMeterIdPrefixFunction and supporting test infrastructure.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good once CI passes


final ClientRequestContext ctx;
try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) {
final ListenableFuture<ExtendedTestMessage> future =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unfortunately fails due to how shadedTest works in our codebase at the moment.
You may need to either:

  • Avoid using *FutureStub in tests
  • Or move the added test to a module without the relocate flag (i.e. :it:grpc:java)

@jrhee17 jrhee17 added the defect label Feb 4, 2026
@jrhee17 jrhee17 added this to the 1.37.0 milestone Feb 4, 2026
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once @jrhee17's comments are addressed

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
`@grpc/src/test/java/com/linecorp/armeria/common/grpc/GrpcMeterIdPrefixFunctionTest.java`:
- Around line 279-289: In unaryCallWithAllDifferentParameterTypes (in
GrpcMeterIdPrefixFunctionTest) stop processing when the Thread.sleep is
interrupted: in the catch block for InterruptedException, after re-setting the
interrupt flag (Thread.currentThread().interrupt()), return immediately so you
do not call responseObserver.onNext/onCompleted after an interruption (which can
race with cancellation/stream closure).

@sallylyen sallylyen force-pushed the grpc-status-from-response-content branch from 7b8c2c2 to 5a79d3f Compare February 5, 2026 03:57
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

grpc-status not recorded in RequestLog when client cancels (RST_STREAM)

4 participants