Skip to content

Commit

Permalink
remove snapshot.run span
Browse files Browse the repository at this point in the history
  • Loading branch information
@surbhigarg92
surbhigarg92 committed Nov 19, 2024
1 parent 6a8a8a0 commit 17f2aa9
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 167 deletions.
43 changes: 5 additions & 38 deletions observability-test/spanner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ describe('EndToEnd', async () => {
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Database.getSnapshot',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
];
const expectedEventNames = [
'Begin Transaction',
Expand Down Expand Up @@ -288,16 +287,15 @@ describe('EndToEnd', async () => {
await transaction!.end();
const expectedSpanNames = [
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Transaction.commit',
'CloudSpanner.Database.runTransaction',
];
const expectedEventNames = [
'Starting stream',
'Transaction Creation Done',
'Starting Commit',
'Commit Done',
...cacheSessionEvents,
'Transaction Creation Done',
];

await verifySpansAndEvents(
Expand All @@ -317,14 +315,13 @@ describe('EndToEnd', async () => {

const expectedSpanNames = [
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Database.runTransactionAsync',
];
const expectedEventNames = [
'Starting stream',
'Transaction Creation Done',
...cacheSessionEvents,
'Using Session',
'Transaction Creation Done',
];
await verifySpansAndEvents(
traceExporter,
Expand All @@ -333,7 +330,7 @@ describe('EndToEnd', async () => {
);
});

it.only('runTransaction with abort', done => {
it.skip('runTransaction with abort', done => {
let attempts = 0;
let rowCount = 0;
const database = newTestDatabase();
Expand All @@ -354,9 +351,7 @@ describe('EndToEnd', async () => {
'CloudSpanner.Database.batchCreateSessions',
'CloudSpanner.SessionPool.createSessions',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Transaction.commit',
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Database.runTransaction',
Expand All @@ -383,7 +378,7 @@ describe('EndToEnd', async () => {
});

Check failure on line 378 in observability-test/spanner.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `····`
});

it('runTransactionAsync with abort', async () => {
it.skip('runTransactionAsync with abort', async () => {
let attempts = 0;
const database = newTestDatabase();
await database.runTransactionAsync((transaction): Promise<number> => {
Expand All @@ -402,10 +397,8 @@ describe('EndToEnd', async () => {
'CloudSpanner.Database.batchCreateSessions',
'CloudSpanner.SessionPool.createSessions',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Transaction.commit',
'CloudSpanner.Database.runTransactionAsync',
];
Expand Down Expand Up @@ -476,7 +469,6 @@ describe('EndToEnd', async () => {
const expectedSpanNames = [
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Dml.runUpdate',
'CloudSpanner.PartitionedDml.runUpdate',
'CloudSpanner.Database.runPartitionedUpdate',
Expand Down Expand Up @@ -635,7 +627,6 @@ describe('ObservabilityOptions injection and propagation', async () => {
const expectedSpanNames = [
'CloudSpanner.Database.getTransaction',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
];
assert.deepStrictEqual(
actualSpanNames,
Expand Down Expand Up @@ -690,7 +681,6 @@ describe('ObservabilityOptions injection and propagation', async () => {
const expectedSpanNames = [
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Dml.runUpdate',
];
assert.deepStrictEqual(
Expand Down Expand Up @@ -799,7 +789,6 @@ describe('ObservabilityOptions injection and propagation', async () => {
const expectedSpanNames = [
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Dml.runUpdate',
'CloudSpanner.Transaction.rollback',
];
Expand Down Expand Up @@ -1352,7 +1341,6 @@ SELECT 1p
'CloudSpanner.Database.batchCreateSessions',
'CloudSpanner.SessionPool.createSessions',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Transaction.commit',
Expand All @@ -1364,19 +1352,6 @@ SELECT 1p
expectedSpanNames,
`span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
);
const spanSnapshotRun = spans[3];
assert.strictEqual(spanSnapshotRun.name, 'CloudSpanner.Snapshot.run');
const wantSpanErr = '6 ALREADY_EXISTS: ' + messageBadInsertAlreadyExistent;
assert.deepStrictEqual(
spanSnapshotRun.status.code,
SpanStatusCode.ERROR,
'Unexpected status code'
);
assert.deepStrictEqual(
spanSnapshotRun.status.message,
wantSpanErr,
'Unexpexcted error message'
);

const databaseBatchCreateSessionsSpan = spans[0];
assert.strictEqual(
Expand Down Expand Up @@ -1405,8 +1380,7 @@ SELECT 1p

// We need to ensure a strict relationship between the spans.
// |-Database.runTransactionAsync |-------------------------------------|
// |-Snapshot.run |------------------------|
// |-Snapshot.runStream |---------------------|
// |-Snapshot.runStream |---------------------|
// |-Transaction.commit |--------|
// |-Snapshot.begin |------|
// |-Snapshot.commit |-----|
Expand All @@ -1427,12 +1401,6 @@ SELECT 1p
'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.
Expand Down Expand Up @@ -1953,7 +1921,6 @@ describe('Traces for ExecuteStream broken stream retries', () => {
'CloudSpanner.Database.batchCreateSessions',
'CloudSpanner.SessionPool.createSessions',
'CloudSpanner.Snapshot.runStream',
'CloudSpanner.Snapshot.run',
'CloudSpanner.Dml.runUpdate',
'CloudSpanner.Snapshot.begin',
'CloudSpanner.Transaction.commit',
Expand Down
102 changes: 1 addition & 101 deletions observability-test/transaction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -331,107 +331,7 @@ describe('Transaction', () => {
fakeStream.emit('end');
});
});

describe('run', () => {
const QUERY = 'SELET * FROM `MyTable`';

let fakeStream;

beforeEach(() => {
fakeStream = new EventEmitter();
sandbox.stub(snapshot, 'runStream').returns(fakeStream);
});

it('without error', done => {
const fakeRows = [{a: 'b'}, {c: 'd'}, {e: 'f'}];

snapshot.run(QUERY, (err, rows) => {
assert.ifError(err);
assert.deepStrictEqual(rows, fakeRows);

const exportResults = extractExportedSpans();
const actualSpanNames = exportResults.spanNames;
const actualEventNames = exportResults.spanEventNames;

const expectedSpanNames = ['CloudSpanner.Snapshot.run'];
assert.deepStrictEqual(
actualSpanNames,
expectedSpanNames,
`span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
);

const expectedEventNames = [];
assert.deepStrictEqual(
actualEventNames,
expectedEventNames,
`Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}`
);

// Ensure that the final span that got retries did not error.
const spans = exportResults.spans;
const firstSpan = spans[0];
assert.strictEqual(
SpanStatusCode.UNSET,
firstSpan.status.code,
'Unexpected an span status code'
);
assert.strictEqual(
undefined,
firstSpan.status.message,
'Unexpected span status message'
);
done();
});

fakeRows.forEach(row => fakeStream.emit('data', row));
fakeStream.emit('end');
});

it('with errors', done => {
const fakeError = new Error('run.error');

snapshot.run(QUERY, err => {
assert.strictEqual(err, fakeError);

const exportResults = extractExportedSpans();
const actualSpanNames = exportResults.spanNames;
const actualEventNames = exportResults.spanEventNames;

const expectedSpanNames = ['CloudSpanner.Snapshot.run'];
assert.deepStrictEqual(
actualSpanNames,
expectedSpanNames,
`span names mismatch:\n\tGot: ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
);

const expectedEventNames = [];
assert.deepStrictEqual(
actualEventNames,
expectedEventNames,
`Unexpected events:\n\tGot: ${actualEventNames}\n\tWant: ${expectedEventNames}`
);

// Ensure that the final span that got retries did not error.
const spans = exportResults.spans;
const firstSpan = spans[0];
assert.strictEqual(
SpanStatusCode.ERROR,
firstSpan.status.code,
'Unexpected an span status code'
);
assert.strictEqual(
'run.error',
firstSpan.status.message,
'Unexpected span status message'
);

done();
});

fakeStream.emit('error', fakeError);
});
});


Check failure on line 334 in observability-test/transaction.ts

View workflow job for this annotation

GitHub Actions / lint 

Delete `····`

Check failure on line 334 in observability-test/transaction.ts

View workflow job for this annotation

GitHub Actions / lint 

Trailing spaces not allowed
describe('runStream', () => {
const QUERY = {
sql: 'SELECT * FROM `MyTable`',
Expand Down
2 changes: 1 addition & 1 deletion src/partial-result-stream.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ export function partialResultStream(
// checkpoint stream has queued. After that, we will destroy the
// user's stream with the same error.
setImmediate(() => batchAndSplitOnTokenStream.destroy(err));
setSpanErrorAndException(span, err as Error);
// setSpanErrorAndException(span, err as Error);
return;
}

Expand Down
1 change: 0 additions & 1 deletion src/transaction-runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ export class TransactionRunner extends Runner<void> {
}

stream.unpipe(proxyStream);
// proxyStream.emit('error', err);
reject(err);
})
.pipe(proxyStream);
Expand Down
42 changes: 16 additions & 26 deletions src/transaction.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1093,33 +1093,23 @@ export class Snapshot extends EventEmitter {
let stats: google.spanner.v1.ResultSetStats;
let metadata: google.spanner.v1.ResultSetMetadata;

const traceConfig = {
sql: query,
opts: this._observabilityOptions,
dbName: this._dbName!,
};
startTrace('Snapshot.run', traceConfig, span => {
return this.runStream(query)
.on('error', (err, rows, stats, metadata) => {
setSpanError(span, err);
span.end();
callback!(err, rows, stats, metadata);
})
.on('response', response => {
if (response.metadata) {
metadata = response.metadata;
if (metadata.transaction && !this.id) {
this._update(metadata.transaction);
}
this.runStream(query)
.on('error', (err, rows, stats, metadata) => {
callback!(err, rows, stats, metadata);
})
.on('response', response => {
if (response.metadata) {
metadata = response.metadata;
if (metadata.transaction && !this.id) {
this._update(metadata.transaction);
}
})
.on('data', row => rows.push(row))
.on('stats', _stats => (stats = _stats))
.on('end', () => {
span.end();
callback!(null, rows, stats, metadata);
});
});
}
})
.on('data', row => rows.push(row))
.on('stats', _stats => (stats = _stats))
.on('end', () => {
callback!(null, rows, stats, metadata);
});
}

/**
Expand Down

0 comments on commit 17f2aa9

Please sign in to comment.