feat(gax): report response status codes on traced calls - #9344
shivanee-p wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances error reporting and telemetry in the REST fallback transport by capturing and recording the actual HTTP response status code. It introduces an 'httpStatusCode' property to 'GoogleError' and updates the OpenTelemetry tracing helper ('TracerHelper') to extract and attach 'rpc.response.status_code', 'grpc.response.status_code', and 'http.response.status_code' attributes to spans. Comprehensive unit tests have also been added to verify these changes. I have no further feedback to provide as the implementation is robust and well-tested.
6dfbcfb to
20ab67b
Compare
20ab67b to
b5d1c7b
Compare
b5d1c7b to
e81a620
Compare
e81a620 to
3f821e6
Compare
Spans now carry the status of the call they measure. rpc.response.status_code holds the gRPC status name on both transports, since that is the one status gax resolves everywhere and the only value comparable across them. grpc.response.status_code mirrors it on gRPC spans, and http.response.status_code carries the received HTTP status on fallback spans. The HTTP status could not simply be derived from the gRPC code: rpcCodeFromHttpStatusCode collapses whole ranges, so the received status is unrecoverable from the mapping. It is now plumbed from the fetch response through decodeResponse onto GoogleError.httpStatusCode, and is absent when no response arrived at all, such as an expired deadline. Success is inferred rather than observed. The unary path never surfaces a status object to gax, so a call that reported no error is recorded as OK, and 200 on the fallback. Adds OtelHarness.assertResponseStatus, which derives the expected attribute shape from the span's own gcp.method.type and asserts both the presence of the attribute that applies and the absence of the one that does not, so a test cannot assert a combination the tracer should never produce. The two tests that read httpStatusCode back off an error go through setMockFallbackHttpResponse rather than the always-resolving mock, because the status has to survive the same reject/resolve split validateStatus produces in production for the assertion to mean anything. A 500 resolves and is decoded, so the received status is recorded alongside the gRPC code the body maps to; the 503-with-a-400-body case exists because the two have to differ for the assertion to distinguish a recorded status from a derived one.
3f821e6 to
42dc42a
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for capturing and recording the actual HTTP response status code received by the REST fallback transport, storing it on the GoogleError object and reporting it via OpenTelemetry span attributes. Feedback suggests enhancing the resolveRpcStatusName helper in TracerHelper.ts to robustly handle stringified numbers and string status names in addition to numeric codes.
| function resolveRpcStatusName(e: unknown): string { | ||
| const code = (e as {code?: unknown} | null)?.code; | ||
| if ( | ||
| typeof code === 'number' && | ||
| code !== Status.OK && | ||
| Status[code] !== undefined | ||
| ) { | ||
| return Status[code]; | ||
| } | ||
| return Status[Status.UNKNOWN]; | ||
| } |
There was a problem hiding this comment.
The resolveRpcStatusName helper currently only resolves numeric status codes. In JavaScript/TypeScript environments, errors (especially custom or third-party ones) may carry status codes as stringified numbers (e.g., '5') or as string status names (e.g., 'NOT_FOUND'). Supporting both stringified numbers and string status names would make the status resolution significantly more robust.
function resolveRpcStatusName(e: unknown): string {
const code = (e as {code?: unknown} | null)?.code;
if (typeof code === 'number') {
if (code !== Status.OK && Status[code] !== undefined) {
return Status[code];
}
} else if (typeof code === 'string') {
const parsed = parseInt(code, 10);
if (!isNaN(parsed)) {
if (parsed !== Status.OK && Status[parsed] !== undefined) {
return Status[parsed];
}
} else if (code !== 'OK' && typeof (Status as any)[code] === 'number') {
return code;
}
}
return Status[Status.UNKNOWN];
}
Pass through the status code to spans.
rpc.response.status_codeholds the gRPC status name on both transports, since gax resolves it everywhere.grpc.response.status_codemirrors it on gRPC spans, andhttp.response.status_codecarries the received HTTP status on fallback spans.The HTTP status is plumbed from the fetch response through decodeResponse onto
GoogleError.httpStatusCode, and is absent when no response arrived at all, such as an expired deadline.Success is inferred rather than observed. The unary path never surfaces a status object to gax, so a call that reported no error is recorded as OK, and 200 on the fallback.
Adds
OtelHarness.assertResponseStatus, which derives the expected attribute shape from the span's owngcp.method.typeand asserts both the presence of the attribute that applies and the absence of the one that does not, so a test cannot assert a combination the tracer should never produce.