traces tests refactoring

surbhigarg92 · Oct 29, 2024 · 4e85d45 · 4e85d45
1 parent 4b2f196
commit 4e85d45
Show file tree

Hide file tree

Showing 10 changed files with 220 additions and 433 deletions.
diff --git a/observability-test/batch-transaction.ts b/observability-test/batch-transaction.ts
@@ -153,103 +153,95 @@ describe('BatchTransaction', () => {
   };
 
   it('createQueryPartitions', done => {
-    const REQUEST = sandbox.stub();
-
-    const res = batchTransaction.createQueryPartitions(
-      QUERY,
-      (err, part, resp) => {
-        assert.ifError(err);
-        traceExporter.forceFlush();
-        const spans = traceExporter.getFinishedSpans();
-        assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected');
-
-        // Sort the spans by duration.
-        spans.sort((spanA, spanB) => {
-          spanA.duration < spanB.duration;
-        });
-
-        const actualSpanNames: string[] = [];
-        spans.forEach(span => {
-          actualSpanNames.push(span.name);
-        });
-
-        const expectedSpanNames = [
-          'CloudSpanner.BatchTransaction.createPartitions_',
-          'CloudSpanner.BatchTransaction.createQueryPartitions',
-        ];
-        assert.deepStrictEqual(
-          actualSpanNames,
-          expectedSpanNames,
-          `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
-        );
-
-        // Ensure that createPartitions_ is a child span of createQueryPartitions.
-        const spanCreatePartitions_ = spans[0];
-        const spanCreateQueryPartitions = spans[1];
-        assert.ok(
-          spanCreateQueryPartitions.spanContext().traceId,
-          'Expected that createQueryPartitions has a defined traceId'
-        );
-        assert.ok(
-          spanCreatePartitions_.spanContext().traceId,
-          'Expected that createPartitions_ has a defined traceId'
-        );
-        assert.deepStrictEqual(
-          spanCreatePartitions_.spanContext().traceId,
-          spanCreateQueryPartitions.spanContext().traceId,
-          'Expected that both spans share a traceId'
-        );
-        assert.ok(
-          spanCreateQueryPartitions.spanContext().spanId,
-          'Expected that createQueryPartitions has a defined spanId'
-        );
-        assert.ok(
-          spanCreatePartitions_.spanContext().spanId,
-          'Expected that createPartitions_ has a defined spanId'
-        );
-        assert.deepStrictEqual(
-          spanCreatePartitions_.parentSpanId,
-          spanCreateQueryPartitions.spanContext().spanId,
-          'Expected that createQueryPartitions is the parent to createPartitions_'
-        );
-        done();
-      }
-    );
+    const res = batchTransaction.createQueryPartitions(QUERY, err => {
+      assert.ifError(err);
+      traceExporter.forceFlush();
+      const spans = traceExporter.getFinishedSpans();
+      assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected');
+
+      // Sort the spans by duration.
+      spans.sort((spanA, spanB) => {
+        spanA.duration < spanB.duration;
+      });
+
+      const actualSpanNames: string[] = [];
+      spans.forEach(span => {
+        actualSpanNames.push(span.name);
+      });
+
+      const expectedSpanNames = [
+        'CloudSpanner.BatchTransaction.createPartitions_',
+        'CloudSpanner.BatchTransaction.createQueryPartitions',
+      ];
+      assert.deepStrictEqual(
+        actualSpanNames,
+        expectedSpanNames,
+        `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
+      );
+
+      // Ensure that createPartitions_ is a child span of createQueryPartitions.
+      const spanCreatePartitions_ = spans[0];
+      const spanCreateQueryPartitions = spans[1];
+      assert.ok(
+        spanCreateQueryPartitions.spanContext().traceId,
+        'Expected that createQueryPartitions has a defined traceId'
+      );
+      assert.ok(
+        spanCreatePartitions_.spanContext().traceId,
+        'Expected that createPartitions_ has a defined traceId'
+      );
+      assert.deepStrictEqual(
+        spanCreatePartitions_.spanContext().traceId,
+        spanCreateQueryPartitions.spanContext().traceId,
+        'Expected that both spans share a traceId'
+      );
+      assert.ok(
+        spanCreateQueryPartitions.spanContext().spanId,
+        'Expected that createQueryPartitions has a defined spanId'
+      );
+      assert.ok(
+        spanCreatePartitions_.spanContext().spanId,
+        'Expected that createPartitions_ has a defined spanId'
+      );
+      assert.deepStrictEqual(
+        spanCreatePartitions_.parentSpanId,
+        spanCreateQueryPartitions.spanContext().spanId,
+        'Expected that createQueryPartitions is the parent to createPartitions_'
+      );
+      done();
+    });
   });
 
   it('createReadPartitions', done => {
     const REQUEST = sandbox.stub();
     const response = {};
     REQUEST.callsFake((_, callback) => callback(null, response));
 
-    const res = batchTransaction.createReadPartitions(
-      QUERY,
-      (err, part, resp) => {
-        assert.ifError(err);
-        traceExporter.forceFlush();
-        const spans = traceExporter.getFinishedSpans();
-        assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected');
-
-        // Sort the spans by duration.
-        spans.sort((spanA, spanB) => {
-          spanA.duration < spanB.duration;
-        });
-
-        const actualSpanNames: string[] = [];
-        spans.forEach(span => {
-          actualSpanNames.push(span.name);
-        });
-        const expectedSpanNames = [
-          'CloudSpanner.BatchTransaction.createPartitions_',
-          'CloudSpanner.BatchTransaction.createReadPartitions',
-        ];
-        assert.deepStrictEqual(
-          actualSpanNames,
-          expectedSpanNames,
-          `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
-        );
-        done();
-      }
-    );
+    const res = batchTransaction.createReadPartitions(QUERY, err => {
+      assert.ifError(err);
+      traceExporter.forceFlush();
+      const spans = traceExporter.getFinishedSpans();
+      assert.strictEqual(spans.length, 2, 'Exactly 2 spans expected');
+
+      // Sort the spans by duration.
+      spans.sort((spanA, spanB) => {
+        spanA.duration < spanB.duration;
+      });
+
+      const actualSpanNames: string[] = [];
+      spans.forEach(span => {
+        actualSpanNames.push(span.name);
+      });
+      const expectedSpanNames = [
+        'CloudSpanner.BatchTransaction.createPartitions_',
+        'CloudSpanner.BatchTransaction.createReadPartitions',
+      ];
+      assert.deepStrictEqual(
+        actualSpanNames,
+        expectedSpanNames,
+        `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpanNames}`
+      );
+      done();
+    });
   });
 });
diff --git a/observability-test/database.ts b/observability-test/database.ts
@@ -1175,7 +1175,7 @@ describe('Database', () => {
 
     it('with error on null mutation should catch thrown error', done => {
       try {
-        database.writeAtLeastOnce(null, (err, res) => {});
+        database.writeAtLeastOnce(null, () => {});
       } catch (err) {
         // Performing a substring search on the error because
         // depending on the version of Node.js, the error might be either of:
@@ -1320,7 +1320,6 @@ describe('Database', () => {
             'Expected an ERROR span status'
           );
 
-          const errorMessage = firstSpan.status.message;
           assert.deepStrictEqual(
             firstSpan.status.message,
             sessionNotFoundError.message
@@ -1658,7 +1657,7 @@ describe('Database', () => {
         .throws(ourException);
 
       assert.rejects(async () => {
-        const value = await database.runTransactionAsync(async txn => {
+        await database.runTransactionAsync(async txn => {
           const result = await txn.run('SELECT 1');
           await txn.commit();
           return result;

diff --git a/observability-test/helper.ts b/observability-test/helper.ts
@@ -19,6 +19,25 @@ import * as assert from 'assert';
 const {ReadableSpan} = require('@opentelemetry/sdk-trace-base');
 import {SEMATTRS_DB_NAME} from '@opentelemetry/semantic-conventions';
 
+export const batchCreateSessionsEvents = [
+  'Requesting 25 sessions',
+  'Creating 25 sessions',
+  'Requested for 25 sessions returned 25',
+];
+
+export const waitingSessionsEvents = [
+  'Acquiring session',
+  'Waiting for a session to become available',
+  'Acquired session',
+  'Using Session',
+];
+
+export const cacheSessionEvents = [
+  'Acquiring session',
+  'Cache hit: has usable session',
+  'Acquired session',
+];
+
 /**
  * This utility exists as a test helper because mocha has builtin "context"
  * and referring to context causes type/value collision errors.
@@ -47,3 +66,30 @@ export function generateWithAllSpansHaveDBName(dbName: String): Function {
     });
   };
 }
+
+export async function verifySpansAndEvents(
+  traceExporter,
+  expectedSpans,
+  expectedEvents
+) {
+  await traceExporter.forceFlush();
+  const spans = traceExporter.getFinishedSpans();
+  const actualEventNames: string[] = [];
+  const actualSpanNames: string[] = [];
+  spans.forEach(span => {
+    actualSpanNames.push(span.name);
+    span.events.forEach(event => {
+      actualEventNames.push(event.name);
+    });
+  });
+  assert.deepStrictEqual(
+    actualSpanNames,
+    expectedSpans,
+    `span names mismatch:\n\tGot:  ${actualSpanNames}\n\tWant: ${expectedSpans}`
+  );
+  assert.deepStrictEqual(
+    actualEventNames,
+    expectedEvents,
+    `Unexpected events:\n\tGot:  ${actualEventNames}\n\tWant: ${expectedEvents}`
+  );
+}
diff --git a/observability-test/observability.ts b/observability-test/observability.ts
@@ -382,7 +382,6 @@ describe('setError', () => {
 
   it('a non-empty string should set the message', () => {
     startTrace('aSpan', {opts: {tracerProvider: provider}}, span => {
-      const status1 = span.status;
       const res = setSpanError(span, 'this one');
       assert.strictEqual(res, true, 'value was set');
       span.end();
@@ -438,7 +437,6 @@ describe('setErrorAndException', () => {
 
   it('a non-empty string should set the message', () => {
     startTrace('aSpan', {opts: {tracerProvider: provider}}, span => {
-      const status1 = span.status;
       const res = setSpanErrorAndException(span, 'this one');
       assert.strictEqual(res, true, 'value was set');
       span.end();

diff --git a/observability-test/session-pool.ts b/observability-test/session-pool.ts
@@ -65,7 +65,7 @@ describe('SessionPool', () => {
   } as unknown as Database;
 
   const sandbox = sinon.createSandbox();
-  const shouldNotBeCalled = sandbox.stub().throws('Should not be called.');
+  sandbox.stub().throws('Should not be called.');
 
   const createSession = (name = 'id', props?): Session => {
     props = props || {};
@@ -112,9 +112,7 @@ describe('SessionPool', () => {
     const OPTIONS = 3;
     it('on exception from Database.batchCreateSessions', async () => {
       const ourException = new Error('this fails intentionally');
-      const stub = sandbox
-        .stub(DATABASE, 'batchCreateSessions')
-        .throws(ourException);
+      sandbox.stub(DATABASE, 'batchCreateSessions').throws(ourException);
       const releaseStub = sandbox.stub(sessionPool, 'release');
 
       assert.rejects(async () => {
@@ -168,9 +166,7 @@ describe('SessionPool', () => {
     it('without error', async () => {
       const RESPONSE = [[{}, {}, {}]];
 
-      const stub = sandbox
-        .stub(DATABASE, 'batchCreateSessions')
-        .resolves(RESPONSE);
+      sandbox.stub(DATABASE, 'batchCreateSessions').resolves(RESPONSE);
       const releaseStub = sandbox.stub(sessionPool, 'release');
 
       await sessionPool._createSessions(OPTIONS);