From fecaf8c188a5130ca854294ac94c2cd4d36d1804 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Tue, 15 Sep 2026 15:27:41 -0700 Subject: [PATCH 1/5] fix(gax): align span error reporting with OTel semconv Three deviations from the semantic conventions, all in traceCall: - error.message was set on every failed span. semconv deprecated the attribute and calls it NOT RECOMMENDED on spans, because it has unbounded cardinality and restates the span status description that already carries the message. The status description is now its only home. - Successful calls set the span status to OK. semconv requires the status to be left unset when an operation ends without any errors; OK is reserved for an application explicitly overriding the instrumentation's judgement, which a library must never claim on the application's behalf. - A non-Error throw reported no error.type at all, leaving the failure invisible to any error-rate query that groups on it. It now reports the semconv-defined _OTHER fallback. The accompanying exception event is dropped: recordException on a bare string yields an event with no exception.type and no stacktrace, which adds nothing the status description does not already carry. rpc.response.status_code is unaffected. It is a domain-specific RPC status rather than the span status, and semconv recommends reporting a domain-specific attribute alongside error.type. --- .../gax/src/observability/TracerHelper.ts | 31 +++++-- core/packages/gax/test/unit/apiCallable.ts | 7 +- core/packages/gax/test/unit/tracerHelper.ts | 87 ++++++++++--------- 3 files changed, 71 insertions(+), 54 deletions(-) diff --git a/core/packages/gax/src/observability/TracerHelper.ts b/core/packages/gax/src/observability/TracerHelper.ts index 9f06e8c59a6..9c7487f48a4 100644 --- a/core/packages/gax/src/observability/TracerHelper.ts +++ b/core/packages/gax/src/observability/TracerHelper.ts @@ -376,6 +376,11 @@ export function traceCall( // Marks the span failed. Kept separate from recordError so paths that are // failures but not exceptions can set the status without emitting a // misleading exception event. + // + // The human-readable message is reported here and nowhere else. There is + // deliberately no `error.message` attribute: semconv deprecated it and + // calls it NOT RECOMMENDED on spans, because it has unbounded cardinality + // and restates the status description that already carries it. const setErrorStatus = (message: string) => { errorRecorded = true; span.setStatus({code: SpanStatusCode.ERROR, message}); @@ -400,8 +405,14 @@ export function traceCall( span.setAttributes(attributes); }; - // Every path ends here, so the status is resolved in one place: ERROR if - // anything reported a failure, OK otherwise. + // Every path ends here, so the outcome is resolved in one place: ERROR if + // anything reported a failure, and left unset otherwise. + // + // A successful call deliberately does not set OK. Per OTel semconv the + // span status "MUST be left unset if the instrumented operation has ended + // without any errors"; `OK` is reserved for an application explicitly + // overriding the instrumentation's judgement, and a library must never + // claim it on the application's behalf. Unset already reads as success. const endSpan = () => { if (!spanEnded) { spanEnded = true; @@ -411,7 +422,6 @@ export function traceCall( // call, and success means a 2xx, so 200 is the only value available. // A legacy Apiary 204 is therefore also reported as 200. httpStatusCode = 200; - span.setStatus({code: SpanStatusCode.OK}); } setStatusAttributes(); span.end(); @@ -426,7 +436,6 @@ export function traceCall( httpStatusCode = resolveHttpStatusCode(e); if (e instanceof Error) { span.setAttributes({ - 'error.message': e.message, 'error.type': resolveErrorType(e), }); // recordException emits the `exception` event, which carries @@ -436,12 +445,18 @@ export function traceCall( span.recordException(e); setErrorStatus(e.message); } else { - const message = String(e); + // A non-Error throw carries no type, no message and no stack. `_OTHER` + // is the fallback semconv defines for exactly this, and reporting + // something matters: error.type is the dimension error-rate queries + // group on, so a failure missing it is invisible to them. + // + // No exception event is emitted here. recordException on a bare string + // yields an event with no exception.type and no stacktrace, which adds + // nothing the status description does not already carry. span.setAttributes({ - 'error.message': message, + 'error.type': '_OTHER', }); - span.recordException(message); - setErrorStatus(message); + setErrorStatus(String(e)); } }; diff --git a/core/packages/gax/test/unit/apiCallable.ts b/core/packages/gax/test/unit/apiCallable.ts index 92f8abea3fb..7d85bdb2ea0 100644 --- a/core/packages/gax/test/unit/apiCallable.ts +++ b/core/packages/gax/test/unit/apiCallable.ts @@ -948,7 +948,7 @@ describe('createApiCall', () => { assert.strictEqual(spans.length, 1); const span = spans[0]; assert.strictEqual(span.ended, true); - assert.strictEqual(span.attributes['error.message'], 'RPC test failure'); + assert.strictEqual(span.status.message, 'RPC test failure'); assert.strictEqual(span.events.length, 1); assert.strictEqual(span.events[0].name, 'exception'); }); @@ -1157,10 +1157,7 @@ describe('createApiCall', () => { assert.strictEqual(spans.length, 1); const span = spans[0]; assert.strictEqual(span.ended, true); - assert.strictEqual( - span.attributes['error.message'], - 'streaming test failure', - ); + assert.strictEqual(span.status.message, 'streaming test failure'); assert.strictEqual(span.events.length, 1); assert.strictEqual(span.events[0].name, 'exception'); done(); diff --git a/core/packages/gax/test/unit/tracerHelper.ts b/core/packages/gax/test/unit/tracerHelper.ts index 65aa37a548f..c1b11e61354 100644 --- a/core/packages/gax/test/unit/tracerHelper.ts +++ b/core/packages/gax/test/unit/tracerHelper.ts @@ -107,6 +107,10 @@ describe('TracerHelper', () => { ); assert.strictEqual(span.attributes['gcp.method.name'], 'GetObject'); assert.strictEqual(span.attributes['gcp.method.type'], 'grpc'); + // A successful call reports no error.type, and leaves the status unset + // rather than claiming OK on the application's behalf. + assert.strictEqual(span.attributes['error.type'], undefined); + assert.strictEqual(span.status.code, SpanStatusCode.UNSET); assert.strictEqual(span.events.length, 0); }); @@ -132,7 +136,10 @@ describe('TracerHelper', () => { const span = spans[0]; assert.strictEqual(span.name, 'StorageClient.GetObject'); assert.strictEqual(span.ended, true); - assert.strictEqual(span.attributes['error.message'], 'RPC Failed'); + // The message is carried by the status description. `error.message` is + // deprecated and NOT RECOMMENDED on spans, so it must not appear. + assert.strictEqual(span.status.message, 'RPC Failed'); + assert.strictEqual(span.attributes['error.message'], undefined); // No status code on this error, so error.type falls back to the class. assert.strictEqual(span.attributes['error.type'], 'Error'); // exception.* belongs on the exception event, not on the span. @@ -260,7 +267,7 @@ describe('TracerHelper', () => { assert.strictEqual(span.attributes['error.type'], 'GoogleError'); }); - it('omits error.type entirely when a non-Error is thrown', async () => { + it('reports the _OTHER error.type when a non-Error is thrown', async () => { await assert.rejects(async () => { await traceCall(dynamicArgs, staticArgs, async () => { throw 'plain string failure'; @@ -268,13 +275,16 @@ describe('TracerHelper', () => { }); const span = harness.requireSingleSpan('google-gax'); - assert.strictEqual( - span.attributes['error.message'], - 'plain string failure', - ); - assert.strictEqual(span.attributes['error.type'], undefined); + // Something must be reported, or the failure is invisible to any + // error-rate query that groups on error.type. + assert.strictEqual(span.attributes['error.type'], '_OTHER'); assert.strictEqual(span.attributes['exception.type'], undefined); assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + // The thrown value survives as the status description. + assert.strictEqual(span.status.message, 'plain string failure'); + // No exception event: recordException on a bare string yields one with + // no type and no stacktrace, which adds nothing to the status above. + assert.strictEqual(span.events.length, 0); }); it('handles missing optional static arguments gracefully', async () => { @@ -545,15 +555,18 @@ describe('TracerHelper', () => { cancel(): void {} then( onfulfilled?: - ((value: string) => TResult1 | PromiseLike) | null, + | ((value: string) => TResult1 | PromiseLike) + | null, onrejected?: - ((reason: unknown) => TResult2 | PromiseLike) | null, + | ((reason: unknown) => TResult2 | PromiseLike) + | null, ): Promise { return this.promise.then(onfulfilled, onrejected); } catch( onrejected?: - ((reason: unknown) => TResult | PromiseLike) | null, + | ((reason: unknown) => TResult | PromiseLike) + | null, ): Promise { return this.promise.catch(onrejected); } @@ -618,10 +631,7 @@ describe('TracerHelper', () => { const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); assert.strictEqual(spans[0].ended, true); - assert.strictEqual( - spans[0].attributes['error.message'], - 'ongoing call failed', - ); + assert.strictEqual(spans[0].status.message, 'ongoing call failed'); }); it('ends span synchronously if result is not a Promise', () => { @@ -653,10 +663,7 @@ describe('TracerHelper', () => { const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); assert.strictEqual(spans[0].ended, true); - assert.strictEqual( - spans[0].attributes['error.message'], - 'async promise failure', - ); + assert.strictEqual(spans[0].status.message, 'async promise failure'); assert.strictEqual(spans[0].events.length, 1); }); @@ -698,10 +705,7 @@ describe('TracerHelper', () => { const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); assert.strictEqual(spans[0].ended, true); - assert.strictEqual( - spans[0].attributes['error.message'], - 'stream failure', - ); + assert.strictEqual(spans[0].status.message, 'stream failure'); assert.strictEqual(spans[0].events.length, 1); assert.strictEqual(spans[0].events[0].name, 'exception'); }); @@ -810,7 +814,7 @@ describe('TracerHelper', () => { assert.strictEqual(spansAfterAttempt1.length, 1); assert.strictEqual(spansAfterAttempt1[0].ended, true); assert.strictEqual( - spansAfterAttempt1[0].attributes['error.message'], + spansAfterAttempt1[0].status.message, 'transient stream failure', ); assert.strictEqual(spansAfterAttempt1[0].events.length, 1); @@ -912,10 +916,7 @@ describe('TracerHelper', () => { const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); assert.strictEqual(spans[0].ended, true); - assert.strictEqual( - spans[0].attributes['error.message'], - 'RPC Failed', - ); + assert.strictEqual(spans[0].status.message, 'RPC Failed'); assert.strictEqual( spans[0].attributes['exception.type'], undefined, @@ -1078,18 +1079,21 @@ describe('TracerHelper', () => { return spans[0].status; }; - it('sets OK for a synchronous non-promise result', () => { + // A successful call leaves the status UNSET rather than setting OK. + // semconv reserves OK for an application overriding the + // instrumentation's judgement, so a library must never emit it. + it('leaves the status unset for a synchronous non-promise result', () => { traceCall( dynamicArgs, staticArgs, () => ({data: 1}) as unknown as ResultTuple, ); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); - it('sets OK when the promise resolves', async () => { + it('leaves the status unset when the promise resolves', async () => { await traceCall(dynamicArgs, staticArgs, async () => ({data: 1})); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); it('sets ERROR when the promise rejects', async () => { @@ -1114,11 +1118,11 @@ describe('TracerHelper', () => { assert.strictEqual(status.message, 'sync boom'); }); - it('sets OK when the stream ends cleanly', () => { + it('leaves the status unset when the stream ends cleanly', () => { const emitter = new EventEmitter(); traceCall(dynamicArgs, staticArgs, () => emitter, true); emitter.emit('end'); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); it('sets ERROR when the stream errors', () => { @@ -1130,7 +1134,7 @@ describe('TracerHelper', () => { assert.strictEqual(status.message, 'stream boom'); }); - it('sets OK when a client-streaming call finishes', async () => { + it('leaves the status unset when a client-streaming call finishes', async () => { const writable = new Writable({ objectMode: true, write(_chunk, _enc, cb) { @@ -1141,10 +1145,10 @@ describe('TracerHelper', () => { writable.end(); await new Promise(resolve => setImmediate(resolve)); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); - it('sets OK when the callback reports success', () => { + it('leaves the status unset when the callback reports success', () => { let invokedCallback: APICallback | undefined; traceCall( dynamicArgs, @@ -1157,7 +1161,7 @@ describe('TracerHelper', () => { () => {}, ); invokedCallback!(null, {ok: true}); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); it('sets ERROR when the callback reports failure', () => { @@ -1178,8 +1182,9 @@ describe('TracerHelper', () => { assert.strictEqual(status.message, 'callback boom'); }); - it('does not downgrade an ERROR status to OK when the span ends', () => { - // endSpan resolves the status centrally; a recorded error must win. + it('does not clear an ERROR status when the span ends', () => { + // endSpan resolves the outcome centrally; a recorded error must win + // over the success path, which would otherwise leave it UNSET. const emitter = new EventEmitter(); traceCall(dynamicArgs, staticArgs, () => emitter, true); emitter.emit('error', new Error('stream boom')); @@ -1211,13 +1216,13 @@ describe('TracerHelper', () => { ); invokedCallback!(null, {ok: true}); - assert.strictEqual(lastStatus().code, SpanStatusCode.OK); + assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); emitter.emit('error', new Error('too late')); const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); - assert.strictEqual(spans[0].status.code, SpanStatusCode.OK); + assert.strictEqual(spans[0].status.code, SpanStatusCode.UNSET); // No phantom exception event tacked onto the finished span. assert.strictEqual( spans[0].events.filter(e => e.name === 'exception').length, From a3640dfca1c0c14d2ba119b4c3067473fd322a53 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Tue, 15 Sep 2026 15:35:32 -0700 Subject: [PATCH 2/5] test(gax): cover the error and exception reporting contract The previous commit changed which signal carries what, but the existing tests only assert each attribute where it happens to be used. Nothing pinned the split itself, so the deprecated attribute or an instrumented OK status could return without a single failure. Adds a suite covering the contract directly: - error information (status + error.type) and exception information (the event) stay on their own signal, with neither leaking onto the other - error.message is never set, while the message stays reachable via the status description and the exception event - the exception event carries a stacktrace, the one detail no span attribute may hold - error.type agrees with the RPC status resolved for the same call - exactly one exception event is recorded however many completion signals a stream emits - a non-Error, a coded non-Error and a thrown null all still produce a usable error.type and RPC status - a successful call reports no error information at all The first case also documents that OTel derives exception.type from an error's code property before its name property, so a coded gax error reports '5' on the event where the span reports 'NOT_FOUND'. That asymmetry is the clearest argument for resolving error.type separately. --- core/packages/gax/test/unit/tracerHelper.ts | 147 ++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/core/packages/gax/test/unit/tracerHelper.ts b/core/packages/gax/test/unit/tracerHelper.ts index c1b11e61354..9ca9056f4bb 100644 --- a/core/packages/gax/test/unit/tracerHelper.ts +++ b/core/packages/gax/test/unit/tracerHelper.ts @@ -287,6 +287,153 @@ describe('TracerHelper', () => { assert.strictEqual(span.events.length, 0); }); + // The two signals answer different questions, and the split between them + // is the part most easily broken by a well-meaning edit. Error information + // (span status + error.type) says how the operation ended and is what + // error-rate queries group on, so it must stay low-cardinality and must + // exist for every failure. Exception information (the `exception` event) + // says what was thrown, carries the unbounded detail, and only exists when + // something actually was thrown. + describe('error and exception reporting', () => { + const failWith = async (thrown: unknown) => { + await assert.rejects(async () => { + await traceCall(dynamicArgs, staticArgs, async () => { + throw thrown; + }); + }); + return harness.requireSingleSpan('google-gax'); + }; + + it('keeps error information on the span and exception detail on the event', async () => { + const error = new GoogleError('object does not exist'); + error.code = Status.NOT_FOUND; + + const span = await failWith(error); + + // Error information: the outcome, on the span itself. + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.status.message, 'object does not exist'); + assert.strictEqual(span.attributes['error.type'], 'NOT_FOUND'); + + // Exception information: the detail, on the event. + assert.strictEqual(span.events.length, 1); + const event = span.events[0]; + assert.strictEqual(event.name, 'exception'); + // OTel derives exception.type from `code` when the error carries one, + // falling back to `name` otherwise, so a coded gax error reports the + // bare number '5' here. That is precisely why error.type is resolved + // separately: 'NOT_FOUND' above is the value worth querying on. + assert.strictEqual(event.attributes?.['exception.type'], '5'); + assert.strictEqual( + event.attributes?.['exception.message'], + 'object does not exist', + ); + + // Neither may leak into the other. exception.* on the span would + // duplicate the event at span cardinality, and error.* on the event + // would split the dimension that error-rate queries group on. + assert.strictEqual(span.attributes['exception.type'], undefined); + assert.strictEqual(span.attributes['exception.message'], undefined); + assert.strictEqual(span.attributes['exception.stacktrace'], undefined); + assert.strictEqual(event.attributes?.['error.type'], undefined); + }); + + it('never sets the deprecated error.message attribute', async () => { + // semconv deprecated it and calls it NOT RECOMMENDED on spans: it has + // unbounded cardinality and restates the status description. The + // message must be reachable, just not from here. + const span = await failWith(new Error('quota exceeded')); + + assert.strictEqual(span.attributes['error.message'], undefined); + assert.strictEqual(span.status.message, 'quota exceeded'); + assert.strictEqual( + span.events[0].attributes?.['exception.message'], + 'quota exceeded', + ); + }); + + it('carries the stacktrace on the exception event', async () => { + // The stacktrace is the reason the event exists at all: it is the one + // piece of detail no span attribute is allowed to hold. + const span = await failWith(new Error('boom')); + + const stacktrace = span.events[0].attributes?.['exception.stacktrace']; + assert.strictEqual(typeof stacktrace, 'string'); + assert.ok( + (stacktrace as string).includes('boom'), + `expected a stacktrace mentioning the failure, got ${JSON.stringify( + stacktrace, + )}`, + ); + }); + + it('reports error.type consistently with the RPC status', async () => { + // semconv asks that error.type be applied consistently across the + // signals a single operation reports. The two are resolved by separate + // helpers, so nothing but a test keeps them from drifting apart. + const error = Object.assign(new Error('5 NOT_FOUND: gone'), {code: 5}); + + const span = await failWith(error); + + assert.strictEqual(span.attributes['error.type'], 'NOT_FOUND'); + harness.assertResponseStatus({rpcStatus: 'NOT_FOUND'}, {span}); + }); + + it('records one exception event however many completion signals arrive', async () => { + // A stream can report 'error' and then still emit 'end' and 'close'. + // Only the first may be recorded: a second event would double-count + // the failure, and a later success signal must not overwrite it. + const emitter = new EventEmitter(); + traceCall(dynamicArgs, staticArgs, () => emitter, true); + + emitter.emit('error', new Error('stream broke')); + emitter.emit('end'); + emitter.emit('close'); + + const span = harness.requireSingleSpan('google-gax'); + assert.strictEqual(span.events.length, 1); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.status.message, 'stream broke'); + assert.strictEqual(span.attributes['error.type'], 'Error'); + }); + + it('still resolves the RPC status for a non-Error carrying a code', async () => { + // error.type falls back to _OTHER because a non-Error has no class + // worth reporting, but the domain status is resolved independently and + // is still recoverable. The two do not have to agree here. + const span = await failWith({code: Status.NOT_FOUND}); + + assert.strictEqual(span.attributes['error.type'], '_OTHER'); + harness.assertResponseStatus({rpcStatus: 'NOT_FOUND'}, {span}); + assert.strictEqual(span.events.length, 0); + }); + + it('survives a thrown null', async () => { + // resolveRpcStatusName and String() both have to tolerate it; a throw + // inside recordError would lose the span entirely. + const span = await failWith(null); + + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.status.message, 'null'); + assert.strictEqual(span.attributes['error.type'], '_OTHER'); + harness.assertResponseStatus({rpcStatus: 'UNKNOWN'}, {span}); + }); + + it('reports no error information at all when the call succeeds', async () => { + await traceCall(dynamicArgs, staticArgs, async () => ({ok: true})); + + const span = harness.requireSingleSpan('google-gax'); + // semconv: instrumentation SHOULD NOT set error.type on success, and + // the status MUST be left unset. An UNSET status with no error.type is + // what lets a consumer filter failures out cleanly. + assert.strictEqual(span.attributes['error.type'], undefined); + assert.strictEqual(span.attributes['error.message'], undefined); + assert.strictEqual(span.status.code, SpanStatusCode.UNSET); + assert.strictEqual(span.status.message, undefined); + assert.strictEqual(span.events.length, 0); + }); + }); + it('handles missing optional static arguments gracefully', async () => { const emptyStaticArgs: StaticTraceContext = {}; const result = await traceCall(dynamicArgs, emptyStaticArgs, async () => { From c0dc6a525cefa048ee3f4e75b058ee580f317e50 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Thu, 17 Sep 2026 16:11:57 -0700 Subject: [PATCH 3/5] refactor(gax): compose abort signals without AbortSignal.any Forward the deadline and server-stream cancel signals onto the request's cancel controller by hand, and make the request with that controller's signal, instead of composing them with AbortSignal.any. Behaviour is unchanged: both paths still set their bookkeeping flag before aborting, so the handlers can tell a deadline expiry apart from a cancel(). AbortSignal.any is safe on the supported Node versions, but this module is also the browser entry point and it is a recent addition there (Chrome 116, Safari 17.4). --- core/packages/gax/src/fallbackServiceStub.ts | 41 ++++++++++++-------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/core/packages/gax/src/fallbackServiceStub.ts b/core/packages/gax/src/fallbackServiceStub.ts index 0722ecbd43f..c1b96a382e7 100644 --- a/core/packages/gax/src/fallbackServiceStub.ts +++ b/core/packages/gax/src/fallbackServiceStub.ts @@ -311,14 +311,26 @@ export function generateServiceStub( // difference is bookkeeping: both a deadline expiry and a `cancel()` // abort the same request and surface the same error, so unless we record // which one fired, the handlers below cannot tell them apart. + // + // The request is always made with `cancelSignal`; every other signal is + // forwarded onto `cancelController` by hand. `AbortSignal.any` would + // express this more directly, but it is a recent addition to browsers + // (Chrome 116, Safari 17.4) and this module is also the browser entry + // point, so the composition is done by hand instead. let timedOut = false; - const requestSignals: AbortSignal[] = [cancelSignal]; if (timeoutMs !== undefined) { const timeoutSignal = AbortSignal.timeout(timeoutMs); - timeoutSignal.addEventListener('abort', () => (timedOut = true), { - once: true, - }); - requestSignals.push(timeoutSignal); + timeoutSignal.addEventListener( + 'abort', + () => { + // Set before aborting: the abort synchronously unwinds into the + // handlers below, which read this flag to tell a deadline expiry + // apart from a `cancel()`. + timedOut = true; + cancelController.abort(); + }, + {once: true}, + ); } // A server-streaming call is handed the parser itself rather than a @@ -327,23 +339,20 @@ export function generateServiceStub( // nothing ever listened to it — the request was made with `cancelSignal` // above — so a cancel ended the local stream and left the request in // flight, and the pipeline then reported the resulting teardown as a - // spurious 'Premature close' error. Compose the two signals, and record - // the cancel the way the unary canceller does so that the handlers below - // recognise it as one. + // spurious 'Premature close' error. Forward that cancel onto the request, + // and record it the way the unary canceller does so that the handlers + // below recognise it as one. if (rpc.responseStream) { streamArrayParser.cancelSignal.addEventListener( 'abort', - () => (cancelRequested = true), + () => { + cancelRequested = true; + cancelController.abort(); + }, {once: true}, ); - requestSignals.push(streamArrayParser.cancelSignal); } - const requestSignal = - requestSignals.length === 1 - ? cancelSignal - : AbortSignal.any(requestSignals); - const url = fetchParameters.url; const headers = new Headers(fetchParameters.headers); // gRPC metadata is multi-valued, and `buildMetadata` normalizes every @@ -371,7 +380,7 @@ export function generateServiceStub( ? fetchParameters.body : Buffer.from(fetchParameters.body), method: fetchParameters.method, - signal: requestSignal, + signal: cancelSignal, responseType: 'stream', // ensure gaxios returns the data directly so that it handle data/streams itself // Error responses must resolve so that they are decoded below into a // GoogleError carrying a gRPC status code. 401 and 403 keep rejecting From 88923939633eaa96239373c83314473b2e0323d0 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Thu, 17 Sep 2026 16:36:28 -0700 Subject: [PATCH 4/5] chore: fix prettier formatting in gax tracerHelper test and fallbackServiceStub --- core/packages/gax/test/unit/tracerHelper.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/core/packages/gax/test/unit/tracerHelper.ts b/core/packages/gax/test/unit/tracerHelper.ts index 9ca9056f4bb..6cce23d82cd 100644 --- a/core/packages/gax/test/unit/tracerHelper.ts +++ b/core/packages/gax/test/unit/tracerHelper.ts @@ -702,18 +702,15 @@ describe('TracerHelper', () => { cancel(): void {} then( onfulfilled?: - | ((value: string) => TResult1 | PromiseLike) - | null, + ((value: string) => TResult1 | PromiseLike) | null, onrejected?: - | ((reason: unknown) => TResult2 | PromiseLike) - | null, + ((reason: unknown) => TResult2 | PromiseLike) | null, ): Promise { return this.promise.then(onfulfilled, onrejected); } catch( onrejected?: - | ((reason: unknown) => TResult | PromiseLike) - | null, + ((reason: unknown) => TResult | PromiseLike) | null, ): Promise { return this.promise.catch(onrejected); } From e0092652a2ea054fa9b9127a7ec1d7c4202d0ec4 Mon Sep 17 00:00:00 2001 From: Shivanee Persaud Date: Thu, 17 Sep 2026 17:00:24 -0700 Subject: [PATCH 5/5] feat(gax): describe error-shaped non-Errors from their message property The span status description is the only place a failure's message lives, but a non-Error went through String(), so an object carrying a perfectly good message reported '[object Object]'. resolveRpcStatusName and resolveHttpStatusCode already read their fields off whatever was thrown rather than off an Error; read the message the same way, falling back to String() when there is no string message to use. Also adds status description coverage for the paths that had none: the write-only (client-streaming) failure route, an Error with an empty message, a second distinct failure not overwriting the first description, and a late error leaving the description unset. --- .../gax/src/observability/TracerHelper.ts | 40 ++++++-- core/packages/gax/test/unit/tracerHelper.ts | 96 +++++++++++++++++++ 2 files changed, 128 insertions(+), 8 deletions(-) diff --git a/core/packages/gax/src/observability/TracerHelper.ts b/core/packages/gax/src/observability/TracerHelper.ts index 9c7487f48a4..8d0262f3675 100644 --- a/core/packages/gax/src/observability/TracerHelper.ts +++ b/core/packages/gax/src/observability/TracerHelper.ts @@ -138,6 +138,25 @@ function resolveHttpStatusCode(e: unknown): number | undefined { return typeof code === 'number' ? code : undefined; } +/** + * Resolves the human-readable description for a failure that is not an Error. + * + * `code` and `httpStatusCode` above are read off whatever was thrown rather + * than off an `Error`, because being handed an error-shaped non-Error is a + * real occurrence: a value crossing a realm boundary fails `instanceof` (the + * same hazard `isPromiseLike` documents below), and so does a plain object + * built by a custom transport. `message` is read the same way and for the same + * reason — `String()` on such an object yields '[object Object]', discarding a + * description that was right there. + * + * Anything without a string `message` falls back to `String(e)`, which is all + * a bare string, number, `null` or `undefined` can offer. + */ +function resolveErrorMessage(e: unknown): string { + const message = (e as {message?: unknown} | null)?.message; + return typeof message === 'string' ? message : String(e); +} + /** * Checks if a value behaves like a Promise or Thenable. * @@ -445,18 +464,23 @@ export function traceCall( span.recordException(e); setErrorStatus(e.message); } else { - // A non-Error throw carries no type, no message and no stack. `_OTHER` - // is the fallback semconv defines for exactly this, and reporting - // something matters: error.type is the dimension error-rate queries - // group on, so a failure missing it is invisible to them. + // A non-Error throw has no class worth reporting, so error.type falls + // back to `_OTHER`, the value semconv defines for exactly this. + // Reporting something matters: error.type is the dimension error-rate + // queries group on, so a failure missing it is invisible to them. + // + // The description is still resolved from a `message` property when one + // is there, so an error-shaped object is not reduced to + // '[object Object]'. // - // No exception event is emitted here. recordException on a bare string - // yields an event with no exception.type and no stacktrace, which adds - // nothing the status description does not already carry. + // No exception event is emitted here. recordException on a value that + // is not an Error yields an event with no exception.type and no + // stacktrace, which adds nothing the status description does not + // already carry. span.setAttributes({ 'error.type': '_OTHER', }); - setErrorStatus(String(e)); + setErrorStatus(resolveErrorMessage(e)); } }; diff --git a/core/packages/gax/test/unit/tracerHelper.ts b/core/packages/gax/test/unit/tracerHelper.ts index 6cce23d82cd..26764a0d64e 100644 --- a/core/packages/gax/test/unit/tracerHelper.ts +++ b/core/packages/gax/test/unit/tracerHelper.ts @@ -15,6 +15,7 @@ */ import * as assert from 'assert'; +import * as vm from 'vm'; import {EventEmitter} from 'events'; import {Duplex, Writable} from 'stream'; import {SpanStatusCode} from '@opentelemetry/api'; @@ -406,6 +407,80 @@ describe('TracerHelper', () => { assert.strictEqual(span.attributes['error.type'], '_OTHER'); harness.assertResponseStatus({rpcStatus: 'NOT_FOUND'}, {span}); assert.strictEqual(span.events.length, 0); + // Nothing better is available for an object with no message, so the + // String() fallback stands and the status code above is what has to + // carry the meaning. + assert.strictEqual(span.status.message, '[object Object]'); + }); + + it('describes an error-shaped object from its message property', async () => { + // `code` is already read off whatever was thrown rather than off an + // Error; the description is resolved the same way, so an object that + // carries a perfectly good message is not reduced to '[object Object]'. + const span = await failWith({ + code: Status.NOT_FOUND, + message: 'object does not exist', + }); + + assert.strictEqual(span.status.message, 'object does not exist'); + assert.strictEqual(span.attributes['error.type'], '_OTHER'); + harness.assertResponseStatus({rpcStatus: 'NOT_FOUND'}, {span}); + }); + + it('describes an Error that crossed a realm boundary', async () => { + // A value from another realm fails `instanceof Error`, so it takes the + // non-Error branch despite being a real Error. Reading `message` + // directly keeps the description intact; String() would have prefixed + // it with the class name. + const crossRealm = vm.runInNewContext( + "new Error('cross realm boom')", + ) as Error; + assert.strictEqual(crossRealm instanceof Error, false); + + const span = await failWith(crossRealm); + + assert.strictEqual(span.status.message, 'cross realm boom'); + }); + + it('falls back to String() for a message that is not a string', async () => { + // A non-string `message` is not a description, and passing it through + // would hand OTel a value its status API does not accept. + const span = await failWith({message: {nested: 'object'}}); + + assert.strictEqual(span.status.message, '[object Object]'); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + }); + + it('keeps the description empty when the error has no message', async () => { + // `new Error()` has an empty message, and it must be reported as such. + // Substituting a placeholder would invent a description that no error + // actually carried; the failure is already conveyed by the status code + // and error.type. + const span = await failWith(new Error()); + + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.status.message, ''); + assert.strictEqual(span.attributes['error.type'], 'Error'); + }); + + it('keeps the first description when a second, different failure arrives', async () => { + // The first failure is the one that ended the call; a later error is + // fallout from the teardown. Overwriting the description would replace + // the cause with its symptom, which is the harder direction to debug. + const emitter = new EventEmitter(); + // Attached up front: handleStream removes its own 'error' listener once + // the span is ended, and an EventEmitter with no 'error' listener + // throws on emit. + emitter.on('error', () => {}); + traceCall(dynamicArgs, staticArgs, () => emitter, true); + + emitter.emit('error', new Error('connection reset')); + emitter.emit('error', new Error('premature close')); + + const span = harness.requireSingleSpan('google-gax'); + assert.strictEqual(span.status.code, SpanStatusCode.ERROR); + assert.strictEqual(span.status.message, 'connection reset'); + assert.strictEqual(span.events.length, 1); }); it('survives a thrown null', async () => { @@ -1292,6 +1367,24 @@ describe('TracerHelper', () => { assert.strictEqual(lastStatus().code, SpanStatusCode.UNSET); }); + it('sets ERROR when a client-streaming call fails', () => { + // The write-only path reaches endSpan through 'finish' rather than + // 'end', so its failure route is separate from the readable one above + // and needs its own description assertion. + const writable = new Writable({ + objectMode: true, + write(_chunk, _enc, cb) { + cb(); + }, + }); + traceCall(dynamicArgs, staticArgs, () => writable, true); + writable.emit('error', new GoogleError('upload aborted')); + + const status = lastStatus(); + assert.strictEqual(status.code, SpanStatusCode.ERROR); + assert.strictEqual(status.message, 'upload aborted'); + }); + it('leaves the status unset when the callback reports success', () => { let invokedCallback: APICallback | undefined; traceCall( @@ -1367,6 +1460,9 @@ describe('TracerHelper', () => { const spans = harness.getSpans('google-gax'); assert.strictEqual(spans.length, 1); assert.strictEqual(spans[0].status.code, SpanStatusCode.UNSET); + // The late failure must not leave a description behind either: a + // described UNSET status reads as a success that also failed. + assert.strictEqual(spans[0].status.message, undefined); // No phantom exception event tacked onto the finished span. assert.strictEqual( spans[0].events.filter(e => e.name === 'exception').length,