fix(gax): update error messages to be set to span.status.message

core/packages/gax/src/fallbackServiceStub.ts

@@ -311,14 +311,26 @@
       // 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 @@
       // 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 @@
             ? 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
@@ -420,7 +429,7 @@
                 // state, as the handlers below do.
                 if (err && (timedOut || !cancelRequested)) {
                   if (callback) {
                     callback(err);
                   }
                   streamArrayParser.emit('error', err);
                 }
@@ -435,7 +444,7 @@
               Promise.resolve(response.ok),
               response.arrayBuffer(),
             ])
               .then(([ok, buffer]: [boolean, Buffer | ArrayBuffer]) => {
                 const response = responseDecoder(
                   rpc,
                   ok,
@@ -445,7 +454,7 @@
                 callback!(null, response);
                 return;
               })
               .catch((err: Error) => {
                 // The deadline can expire after the response headers arrive but
                 // before the body is fully read, which rejects here rather than
                 // in the outer handler.
@@ -468,7 +477,7 @@
                   // state we recorded.
                   if (timedOut || !cancelRequested) {
                     if (callback) {
                       callback(callErr);
                     }
                     streamArrayParser.emit('error', callErr);
                   }
@@ -526,12 +535,12 @@
             // nobody is listening to any more.
             if (timedOut || !cancelRequested) {
               if (callback) {
                 callback(err);
               }
               streamArrayParser.emit('error', err);
             }
           } else if (callback) {
             callback(err);
           } else {
             throw err;
           }

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.
  *
@@ -376,6 +395,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 +424,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 +441,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 +455,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 +464,23 @@ export function traceCall(
         span.recordException(e);
         setErrorStatus(e.message);
       } else {
-        const message = String(e);
+        // 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 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.message': message,
+          'error.type': '_OTHER',
         });
-        span.recordException(message);
-        setErrorStatus(message);
+        setErrorStatus(resolveErrorMessage(e));
       }
     };
 

core/packages/gax/test/unit/apiCallable.ts

@@ -948,7 +948,7 @@
       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 @@
           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();
@@ -1191,9 +1188,9 @@
         assert.ok(Array.isArray(response));
         assert.strictEqual(response[0], 42);
         assert.ok(deadlineArg);
         return done();
       })
       .catch(done);
   });
 
   it('emits error on rejected promise', async () => {
@@ -1221,12 +1218,12 @@
     const promise = (apiCall as any)(null);
     promise
       .then(() => {
         return done(new Error('should not reach'));
       })
       .catch((err: {code: number}) => {
         assert(err instanceof GoogleError);
         assert.strictEqual(err.code, status.CANCELLED);
         done();
       });
     promise.cancel();
   });