Transaction.commit and Transaction.createReadStream unconditionally retry by invoke Transaction.begin() if any non-ABORTED error is returned; really it should firstly check for error better

[odeke-em]

Discovered while implementing tracing for this package that we have this code https://github.com/googleapis/nodejs-spanner/blob/0342e74721a0684d8195a6299c3a634eefc2b522/src/transaction.ts#L1321-L1334
which when interpreted will always invoke Transaction.begin() as long this was the transaction was not previously run aka doesn't have an id and the error returned in .commit() was not ABORTED; but really if we just tried to insert data that already exists and the gRPC backend returns 6 ALREADY_EXISTS: Failed to insert row with primary key ({pk#SingerId:100}) due to previously existing row
we shall ALWAYS try to invoke .begin() which then will again invoke .commit()

Reproducing code

'use strict';
const {
  NodeTracerProvider,
  TraceIdRatioBasedSampler,
} = require('@opentelemetry/sdk-trace-node');
const {BatchSpanProcessor} = require('@opentelemetry/sdk-trace-base');

const {
  TraceExporter,
} = require('@google-cloud/opentelemetry-cloud-trace-exporter');
const exporter = new TraceExporter();

const provider = new NodeTracerProvider({
  sampler: new TraceIdRatioBasedSampler(1.0),
});
provider.addSpanProcessor(new BatchSpanProcessor(exporter));

async function main(
  projectId = 'my-project-id',
  instanceId = 'my-instance-id',
  databaseId = 'my-project-id'
) {
  // Create the Cloud Spanner Client.
  const {Spanner} = require('@google-cloud/spanner');
  const spanner = new Spanner({
    projectId: projectId,
    observabilityOptions: {
      tracerProvider: provider,
      enableExtendedTracing: true,
    },
  });

  const instance = spanner.instance(instanceId);
  const database = instance.database(databaseId);

  const update = {
    sql: "INSERT INTO Singers(firstName, SingerId) VALUES('Foo', 100)",
  };
  database.runTransaction(async (err, transaction) => {
    if (err) {
      console.error(err);
      return;
    }
    try {
      await transaction.run(update);
    } catch (err) {
      console.error(err);
    } finally {
      await transaction.commit();
      await new Promise(resolve => {
        setTimeout(() => {
          resolve();
        }, 2000);
      });
      provider.forceFlush();
      // This sleep gives ample time for the trace
      // spans to be exported to Google Cloud Trace.
      await new Promise(resolve => {
        setTimeout(() => {
          resolve();
        }, 4000);
      });
      spanner.close();
    }
  });
}

process.on('unhandledRejection', err => {
  console.error(err.message);
  process.exitCode = 1;
});
main(...process.argv.slice(2));

and here is a trace of the problems to confirm visualization

Currently wrong

Expected

I'd classify as a P0 because this adds extra calls and latency to customer calls on the most minor inconvenience like just a typo or data existing

Suggestion

We should only invoke Transaction.begin() on errors that are retryable for example a transient error or a timeout or server unknown internal errors, deadline_exceeded, status_unavailable, resource_exhaused

Kindly /cc-ing @surbhigarg92 @alkatrivedi