Skip to content

feat(gax): report response status codes on traced calls - #9344

Open
shivanee-p wants to merge 2 commits into
mainfrom
shivaneep-o11y-status-code
Open

shivanee-p wants to merge 2 commits into
mainfrom
shivaneep-o11y-status-code

Conversation

@shivanee-p

Copy link
Copy Markdown
Contributor

Pass through the status code to spans. rpc.response.status_code holds the gRPC status name on both transports, since gax resolves it everywhere. 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 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 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.

@shivanee-p
shivanee-p requested a review from a team as a code owner September 15, 2026 21:10

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@shivanee-p
shivanee-p added this pull request to stack #9345 September 15, 2026 21:12
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-status-code branch from 6dfbcfb to 20ab67b Compare September 15, 2026 21:13
@shivanee-p
shivanee-p marked this pull request as draft September 16, 2026 21:14
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-status-code branch from 20ab67b to b5d1c7b Compare September 17, 2026 17:50
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-status-code branch from b5d1c7b to e81a620 Compare September 17, 2026 18:27
Base automatically changed from shivaneep-o11y-http-tracing to main September 17, 2026 19:50
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-status-code branch from e81a620 to 3f821e6 Compare September 17, 2026 19:50
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.
@shivanee-p
shivanee-p force-pushed the shivaneep-o11y-status-code branch from 3f821e6 to 42dc42a Compare September 17, 2026 23:04
@shivanee-p
shivanee-p marked this pull request as ready for review September 17, 2026 23:05
@shivanee-p

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +121 to +131
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];
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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];
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant