From c62584c861b36516410f41dd8ba426b17f27497a Mon Sep 17 00:00:00 2001 From: surbhigarg92 Date: Thu, 11 Jan 2024 15:17:16 +0530 Subject: [PATCH 1/3] chore: integration test fix --- system-test/spanner.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system-test/spanner.ts b/system-test/spanner.ts index d2bde030f..5575d5c5f 100644 --- a/system-test/spanner.ts +++ b/system-test/spanner.ts @@ -8731,10 +8731,10 @@ describe('Spanner', () => { }, err => { assert.strictEqual(err?.details, expectedErrorMessage); + transaction!.end(); + done(); } ); - transaction!.end(); - done(); }); }); }); From f6fa6672adc146a662c62c43347a5175cf77b8d0 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Sun, 20 Oct 2024 23:00:55 -0700 Subject: [PATCH 2/3] feat: (observability): trace Database.runTransactionAsync Extracted out of PR #2158, this change traces Database.runTransactionAsync. Updates #2079 --- observability-test/database.ts | 2 +- observability-test/spanner.ts | 2 +- src/database.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/observability-test/database.ts b/observability-test/database.ts index 5ce075aa1..6f9230b4a 100644 --- a/observability-test/database.ts +++ b/observability-test/database.ts @@ -1696,7 +1696,7 @@ describe('Database', () => { 'Unexpected span status message' ); - const expectedEventNames = ['Using Session', 'exception']; + const expectedEventNames = ['Using Session']; assert.deepStrictEqual( actualEventNames, expectedEventNames, diff --git a/observability-test/spanner.ts b/observability-test/spanner.ts index 2e90b3044..1716cffc3 100644 --- a/observability-test/spanner.ts +++ b/observability-test/spanner.ts @@ -488,7 +488,7 @@ describe('EndToEnd', async () => { database.formattedName_ ); await database.runTransactionAsync(async transaction => { - await transaction!.run('SELECT 1'); + const [rows] = await transaction!.run('SELECT 1'); }); traceExporter.forceFlush(); diff --git a/src/database.ts b/src/database.ts index 3d8321355..c1ca974d2 100644 --- a/src/database.ts +++ b/src/database.ts @@ -3385,7 +3385,7 @@ class Database extends common.GrpcServiceObject { try { return await runner.run(); } catch (e) { - setSpanErrorAndException(span, e as Error); + setSpanError(span, e as Error); throw e; } finally { span.end(); From f321749de659735d4c851b35bdaae42727fe7282 Mon Sep 17 00:00:00 2001 From: surbhigarg92 Date: Mon, 21 Oct 2024 17:49:55 +0530 Subject: [PATCH 3/3] fix: remove begin call for non retriable errors --- observability-test/database.ts | 2 +- observability-test/spanner.ts | 2 +- src/database.ts | 2 +- src/transaction.ts | 31 --------- test/spanner.ts | 119 +-------------------------------- 5 files changed, 4 insertions(+), 152 deletions(-) diff --git a/observability-test/database.ts b/observability-test/database.ts index 6f9230b4a..5ce075aa1 100644 --- a/observability-test/database.ts +++ b/observability-test/database.ts @@ -1696,7 +1696,7 @@ describe('Database', () => { 'Unexpected span status message' ); - const expectedEventNames = ['Using Session']; + const expectedEventNames = ['Using Session', 'exception']; assert.deepStrictEqual( actualEventNames, expectedEventNames, diff --git a/observability-test/spanner.ts b/observability-test/spanner.ts index 1716cffc3..2e90b3044 100644 --- a/observability-test/spanner.ts +++ b/observability-test/spanner.ts @@ -488,7 +488,7 @@ describe('EndToEnd', async () => { database.formattedName_ ); await database.runTransactionAsync(async transaction => { - const [rows] = await transaction!.run('SELECT 1'); + await transaction!.run('SELECT 1'); }); traceExporter.forceFlush(); diff --git a/src/database.ts b/src/database.ts index c1ca974d2..3d8321355 100644 --- a/src/database.ts +++ b/src/database.ts @@ -3385,7 +3385,7 @@ class Database extends common.GrpcServiceObject { try { return await runner.run(); } catch (e) { - setSpanError(span, e as Error); + setSpanErrorAndException(span, e as Error); throw e; } finally { span.end(); diff --git a/src/transaction.ts b/src/transaction.ts index fa1f10814..5966eb751 100644 --- a/src/transaction.ts +++ b/src/transaction.ts @@ -758,21 +758,6 @@ export class Snapshot extends EventEmitter { this._update(response.metadata!.transaction); } }) - .on('error', err => { - setSpanError(span, err); - const wasAborted = isErrorAborted(err); - if (!this.id && this._useInRunner && !wasAborted) { - // TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170 - this.begin(); - } else { - if (wasAborted) { - span.addEvent('Stream broken. Not safe to retry', { - 'transaction.id': this.id?.toString(), - }); - } - } - span.end(); - }) .on('end', err => { if (err) { setSpanError(span, err); @@ -1349,22 +1334,6 @@ export class Snapshot extends EventEmitter { this._update(response.metadata!.transaction); } }) - .on('error', err => { - setSpanError(span, err as Error); - const wasAborted = isErrorAborted(err); - if (!this.id && this._useInRunner && !wasAborted) { - span.addEvent('Stream broken. Safe to retry'); - // TODO: resolve https://github.com/googleapis/nodejs-spanner/issues/2170 - this.begin(); - } else { - if (wasAborted) { - span.addEvent('Stream broken. Not safe to retry', { - 'transaction.id': this.id?.toString(), - }); - } - } - span.end(); - }) .on('end', err => { if (err) { setSpanError(span, err as Error); diff --git a/test/spanner.ts b/test/spanner.ts index d324ed911..ee95f44ea 100644 --- a/test/spanner.ts +++ b/test/spanner.ts @@ -452,11 +452,8 @@ describe('Spanner with mock server', () => { request.requestOptions!.transactionTag, 'transaction-tag' ); - const beginTxnRequest = spannerMock.getRequests().find(val => { - return (val as v1.BeginTransactionRequest).options?.readWrite; - }) as v1.BeginTransactionRequest; assert.strictEqual( - beginTxnRequest.options?.readWrite!.readLockMode, + request.transaction?.begin?.readWrite?.readLockMode, 'OPTIMISTIC' ); }); @@ -3758,120 +3755,6 @@ describe('Spanner with mock server', () => { ); }); - it('should use beginTransaction on retry for unknown reason', async () => { - const database = newTestDatabase(); - await database.runTransactionAsync(async tx => { - try { - await tx.runUpdate(invalidSql); - assert.fail('missing expected error'); - } catch (e) { - assert.strictEqual( - (e as ServiceError).message, - `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}` - ); - } - await tx.run(selectSql); - await tx.commit(); - }); - await database.close(); - - const beginTxnRequest = spannerMock - .getRequests() - .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite) - .map(req => req as v1.BeginTransactionRequest); - assert.deepStrictEqual(beginTxnRequest.length, 1); - }); - - it('should use beginTransaction on retry for unknown reason with excludeTxnFromChangeStreams', async () => { - const database = newTestDatabase(); - await database.runTransactionAsync( - { - excludeTxnFromChangeStreams: true, - }, - async tx => { - try { - await tx.runUpdate(invalidSql); - assert.fail('missing expected error'); - } catch (e) { - assert.strictEqual( - (e as ServiceError).message, - `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}` - ); - } - await tx.run(selectSql); - await tx.commit(); - } - ); - await database.close(); - - const beginTxnRequest = spannerMock - .getRequests() - .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite) - .map(req => req as v1.BeginTransactionRequest); - assert.deepStrictEqual(beginTxnRequest.length, 1); - assert.strictEqual( - beginTxnRequest[0].options?.excludeTxnFromChangeStreams, - true - ); - }); - - it('should use beginTransaction for streaming on retry for unknown reason', async () => { - const database = newTestDatabase(); - await database.runTransactionAsync(async tx => { - try { - await getRowCountFromStreamingSql(tx!, {sql: invalidSql}); - assert.fail('missing expected error'); - } catch (e) { - assert.strictEqual( - (e as ServiceError).message, - `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}` - ); - } - await tx.run(selectSql); - await tx.commit(); - }); - await database.close(); - - const beginTxnRequest = spannerMock - .getRequests() - .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite) - .map(req => req as v1.BeginTransactionRequest); - assert.deepStrictEqual(beginTxnRequest.length, 1); - }); - - it('should use beginTransaction for streaming on retry for unknown reason with excludeTxnFromChangeStreams', async () => { - const database = newTestDatabase(); - await database.runTransactionAsync( - { - excludeTxnFromChangeStreams: true, - }, - async tx => { - try { - await getRowCountFromStreamingSql(tx!, {sql: invalidSql}); - assert.fail('missing expected error'); - } catch (e) { - assert.strictEqual( - (e as ServiceError).message, - `${grpc.status.NOT_FOUND} NOT_FOUND: ${fooNotFoundErr.message}` - ); - } - await tx.run(selectSql); - await tx.commit(); - } - ); - await database.close(); - - const beginTxnRequest = spannerMock - .getRequests() - .filter(val => (val as v1.BeginTransactionRequest).options?.readWrite) - .map(req => req as v1.BeginTransactionRequest); - assert.deepStrictEqual(beginTxnRequest.length, 1); - assert.strictEqual( - beginTxnRequest[0].options?.excludeTxnFromChangeStreams, - true - ); - }); - it('should fail if beginTransaction fails', async () => { const database = newTestDatabase(); const err = {