Skip to content

Commit 66c5e9a

Browse files
committed
Handle moving 2nd getSnapshot() retry to itself
1 parent 5f6b4d0 commit 66c5e9a

File tree

3 files changed

+11
-19
lines changed

3 files changed

+11
-19
lines changed

observability-test/database.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -605,7 +605,7 @@ describe('Database', () => {
605605
// pool, so that the pool can remove it from its inventory.
606606
const releaseStub = sandbox.stub(fakePool, 'release');
607607

608-
database.getSnapshot((err, snapshot) => {
608+
database.getSnapshot(async (err, snapshot) => {
609609
assert.ifError(err);
610610
assert.strictEqual(snapshot, fakeSnapshot2);
611611
// The first session that error should already have been released back
@@ -616,8 +616,9 @@ describe('Database', () => {
616616
snapshot.emit('end');
617617
assert.strictEqual(releaseStub.callCount, 2);
618618

619+
await provider.forceFlush();
620+
await traceExporter.forceFlush();
619621
const spans = traceExporter.getFinishedSpans();
620-
assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected');
621622
withAllSpansHaveDBName(spans);
622623

623624
const actualSpanNames: string[] = [];

observability-test/spanner.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,6 @@ describe('EndToEnd', () => {
283283
traceExporter.forceFlush();
284284
const spans = traceExporter.getFinishedSpans();
285285
withAllSpansHaveDBName(spans);
286-
console.log(`flushed spans: ${spans.toString()}`);
287286

288287
const actualEventNames: string[] = [];
289288
const actualSpanNames: string[] = [];
@@ -442,10 +441,10 @@ describe('EndToEnd', () => {
442441

443442
database.runTransaction(async (err, transaction) => {
444443
assert.ifError(err);
445-
const [rows] = await transaction!.run('SELECT 1');
444+
await transaction!.run('SELECT 1');
446445
await transaction!.commit();
446+
await traceExporter.forceFlush();
447447

448-
traceExporter.forceFlush();
449448
const spans = traceExporter.getFinishedSpans();
450449
withAllSpansHaveDBName(spans);
451450

@@ -872,7 +871,7 @@ describe('ObservabilityOptions injection and propagation', async () => {
872871
tx!.end();
873872

874873
await tracerProvider.forceFlush();
875-
traceExporter.forceFlush();
874+
await traceExporter.forceFlush();
876875

877876
const spans = traceExporter.getFinishedSpans();
878877
withAllSpansHaveDBName(spans);

src/database.ts

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2105,19 +2105,11 @@ class Database extends common.GrpcServiceObject {
21052105
});
21062106
session!.lastError = err;
21072107
this.pool_.release(session!);
2108-
this.getSnapshot(options, (err, snapshot) => {
2109-
if (err) {
2110-
setSpanError(span, err);
2111-
span.end();
2112-
} else {
2113-
snapshot!.once('end', () => span.end());
2114-
snapshot!.once('error', err => {
2115-
setSpanError(span, err);
2116-
span.end();
2117-
});
2118-
}
2119-
callback!(err, snapshot);
2120-
});
2108+
this.getSnapshot(options, callback!);
2109+
// Explicitly requested in code review that this span.end() be
2110+
// moved out of this.getSnapshot, and that there will a later refactor,
2111+
// similar to https://github.com/googleapis/nodejs-spanner/issues/2159
2112+
span.end();
21212113
} else {
21222114
span.addEvent('Using Session', {'session.id': session?.id});
21232115
this.pool_.release(session!);

0 commit comments

Comments
 (0)