Undo transaction-runner changes plus test vestiges for src/transaction-runner.ts

Author: odeke-em

observability-test/spanner.ts

@@ -453,6 +453,7 @@ describe('EndToEnd', () => {
         const expectedSpanNames = [
           'CloudSpanner.Snapshot.runStream',
           'CloudSpanner.Snapshot.run',
+          'CloudSpanner.Transaction.commit',
           'CloudSpanner.Database.runTransaction',
         ];
         assert.deepStrictEqual(
@@ -464,17 +465,18 @@ describe('EndToEnd', () => {
         const expectedEventNames = [
           'Starting stream',
           'Transaction Creation Done',
+          'Starting Commit',
+          'Commit Done',
           'Acquiring session',
           'Cache hit: has usable session',
           'Acquired session',
-          'Using Session',
-          'Transaction Attempt Succeeded',
         ];
         assert.deepStrictEqual(
           actualEventNames,
           expectedEventNames,
           `Unexpected events:\n\tGot:  ${actualEventNames}\n\tWant: ${expectedEventNames}`
         );
+        done();
       });
     });
 
@@ -1516,37 +1518,8 @@ SELECT 1p
       expectedSpanNames,
       `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
     );
-
-    // We need to ensure a strict relationship between the spans.
-    // |-Database.runTransactionAsync |-------------------------------------|
-    //   |-Snapshot.run                |------------------------|
-    //      |-Snapshot.runStream           |---------------------|
-    //   |-Transaction.commit                                 |--------|
-    //      |-Snapshot.begin                                   |------|
-    //       |-Snapshot.commit                                  |-----|
-    const spanDatabaseRunTransactionAsync = spans[spans.length - 1];
-    assert.deepStrictEqual(
-      spanDatabaseRunTransactionAsync.name,
-      'CloudSpanner.Database.runTransactionAsync',
-      `${actualSpanNames}`
-    );
-    const spanTransactionCommit0 = spans[spans.length - 2];
-    assert.strictEqual(
-      spanTransactionCommit0.name,
-      'CloudSpanner.Transaction.commit'
-    );
-    assert.deepStrictEqual(
-      spanTransactionCommit0.parentSpanId,
-      spanDatabaseRunTransactionAsync.spanContext().spanId,
-      'Expected that Database.runTransaction is the parent to Transaction.commmit'
-    );
     const spanSnapshotRun = spans[2];
     assert.strictEqual(spanSnapshotRun.name, 'CloudSpanner.Snapshot.run');
-    assert.deepStrictEqual(
-      spanSnapshotRun.parentSpanId,
-      spanDatabaseRunTransactionAsync.spanContext().spanId,
-      'Expected that Database.runTransaction is the parent to Snapshot.run'
-    );
     const wantSpanErr = '6 ALREADY_EXISTS: ' + messageBadInsertAlreadyExistent;
     assert.deepStrictEqual(
       spanSnapshotRun.status.code,
@@ -1584,6 +1557,38 @@ SELECT 1p
       'Expected that sessionPool.createSessions is the parent to db.batchCreassionSessions'
     );
 
+    // TODO: Uncomment once we've merged Database.runTransactionAsync tracing.
+    // We need to ensure a strict relationship between the spans.
+    // |-Database.runTransactionAsync |-------------------------------------|
+    //   |-Snapshot.run                |------------------------|
+    //      |-Snapshot.runStream           |---------------------|
+    //   |-Transaction.commit                                 |--------|
+    //      |-Snapshot.begin                                   |------|
+    //       |-Snapshot.commit                                  |-----|
+    /*
+    const spanDatabaseRunTransactionAsync = spans[spans.length - 1];
+    assert.deepStrictEqual(
+      spanDatabaseRunTransactionAsync.name,
+      'CloudSpanner.Database.runTransactionAsync',
+      `${actualSpanNames}`
+    );
+    const spanTransactionCommit0 = spans[spans.length - 1];
+    assert.strictEqual(
+      spanTransactionCommit0.name,
+      'CloudSpanner.Transaction.commit'
+    );
+    assert.deepStrictEqual(
+      spanTransactionCommit0.parentSpanId,
+      spanDatabaseRunTransactionAsync.spanContext().spanId,
+      'Expected that Database.runTransaction is the parent to Transaction.commmit'
+    );
+
+    assert.deepStrictEqual(
+      spanSnapshotRun.parentSpanId,
+      spanDatabaseRunTransactionAsync.spanContext().spanId,
+      'Expected that Database.runTransaction is the parent to Snapshot.run'
+    );
+
     // Assert that despite all being exported, SessionPool.createSessions
     // is not in the same trace as runStream, createSessions is invoked at
     // Spanner Client instantiation, thus before database.run is invoked.
@@ -1592,6 +1597,8 @@ SELECT 1p
       spanDatabaseRunTransactionAsync.spanContext().traceId,
       'Did not expect the same traceId'
     );
+    */
+
     // Finally check for the collective expected event names.
     const expectedEventNames = [
       'Requesting 25 sessions',
@@ -1605,6 +1612,9 @@ SELECT 1p
       'Transaction Creation Done',
       'Starting Commit',
       'Commit Done',
+
+      // TODO: Uncomment once we've merged Database.runTransactionAsync tracing.
+      /*
       'Acquiring session',
       'Waiting for a session to become available',
       'Acquired session',
@@ -1613,6 +1623,7 @@ SELECT 1p
       'Transaction Attempt Aborted',
       'exception',
       'exception',
+     */
     ];
     assert.deepStrictEqual(
       actualEventNames,
@@ -2144,11 +2155,15 @@ describe('Traces for ExecuteStream broken stream retries', () => {
       'Transaction Creation Done',
       'Starting Commit',
       'Commit Done',
+
+      // TODO: Uncomment once we've merged Database.runTransactionAsync tracing.
+      /*
       'Acquiring session',
       'Waiting for a session to become available',
       'Acquired session',
       'Using Session',
       'Transaction Attempt Succeeded',
+     */
     ];
     assert.deepStrictEqual(
       actualEventNames,
@@ -2218,11 +2233,15 @@ describe('Traces for ExecuteStream broken stream retries', () => {
       'Requesting 25 sessions',
       'Creating 25 sessions',
       'Requested for 25 sessions returned 25',
+
+      // TODO: Uncomment once we've merged Database.runTransactionAsync tracing.
+      /*
       'Acquiring session',
       'Waiting for a session to become available',
       'Acquired session',
       'Using Session',
       'Transaction Attempt Succeeded',
+     */
     ];
     assert.deepStrictEqual(
       actualEventNames,

src/transaction-runner.ts

@@ -26,7 +26,6 @@ import {isSessionNotFoundError} from './session-pool';
 import {Database} from './database';
 import {google} from '../protos/protos';
 import IRequestOptions = google.spanner.v1.IRequestOptions;
-import {getActiveOrNoopSpan, setSpanErrorAndException} from './instrument';
 
 // eslint-disable-next-line @typescript-eslint/no-var-requires
 const jsonProtos = require('../protos/protos.json');
@@ -225,7 +224,6 @@ export abstract class Runner<T> {
   async run(): Promise<T> {
     const start = Date.now();
     const timeout = this.options.timeout!;
-    const span = getActiveOrNoopSpan();
 
     let lastError: grpc.ServiceError;
 
@@ -234,21 +232,9 @@ export abstract class Runner<T> {
     while (this.attempts === 0 || Date.now() - start < timeout) {
       const transaction = await this.getTransaction();
 
-      // this.attempts refers to the number of retries, not while loop iterations
-      // hence without a +1, reading a span entry that says attempt=0 makes no
-      // sence to a user.
-      const countableAttempts = this.attempts + 1;
-
       try {
-        const result = await this._run(transaction);
-        span.addEvent('Transaction Attempt Succeeded', {
-          attempt: countableAttempts,
-        });
-        return result;
+        return await this._run(transaction);
       } catch (e) {
-        span.addEvent('Transaction Attempt Failed', {
-          attempt: countableAttempts,
-        });
         this.session.lastError = e as grpc.ServiceError;
         lastError = e as grpc.ServiceError;
       }
@@ -257,29 +243,19 @@ export abstract class Runner<T> {
       // thrown here. We do this to bubble this error up to the caller who is
       // responsible for retrying the transaction on a different session.
       if (
-        !RETRYABLE.includes(lastError!.code!) &&
-        !isRetryableInternalError(lastError!)
+        !RETRYABLE.includes(lastError.code!) &&
+        !isRetryableInternalError(lastError)
       ) {
-        span.addEvent('Transaction Attempt Aborted', {
-          attempt: this.attempts + 1,
-        });
-        setSpanErrorAndException(span, lastError!);
-        throw lastError!;
+        throw lastError;
       }
 
       this.attempts += 1;
 
-      const delay = this.getNextDelay(lastError!);
-      span.addEvent('Backing off', {delay: delay, attempt: countableAttempts});
+      const delay = this.getNextDelay(lastError);
       await new Promise(resolve => setTimeout(resolve, delay));
     }
 
-    span.addEvent('Transaction Attempt Aborted due to Deadline Error', {
-      total_attempts: this.attempts + 1,
-    });
-    const err = new DeadlineError(lastError!);
-    setSpanErrorAndException(span, err);
-    throw err;
+    throw new DeadlineError(lastError!);
   }
 }