From dc404f34b9328773168477d272efc8f285c9b9c5 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 4 Oct 2024 17:16:02 -0700 Subject: [PATCH 1/7] feat: Enable tracing via OpenTelemetry. --- dev/src/index.ts | 3 +- dev/src/telemetry/disabled-trace-util.ts | 4 + dev/src/telemetry/enabled-trace-util.ts | 27 +++- dev/src/telemetry/span.ts | 4 + dev/src/telemetry/trace-util.ts | 13 ++ dev/system-test/tracing.ts | 11 +- dev/test/tracing.ts | 152 ++++++++++++++--------- types/firestore.d.ts | 24 ++++ 8 files changed, 167 insertions(+), 71 deletions(-) diff --git a/dev/src/index.ts b/dev/src/index.ts index 46ad88dfd..5bf9c2217 100644 --- a/dev/src/index.ts +++ b/dev/src/index.ts @@ -792,8 +792,7 @@ export class Firestore implements firestore.Firestore { } private newTraceUtilInstance(settings: firestore.Settings): TraceUtil { - // Take the tracing option from the settings. - let createEnabledInstance = settings.openTelemetryOptions?.enableTracing; + let createEnabledInstance = true; // The environment variable can override options to enable/disable telemetry collection. if ('FIRESTORE_ENABLE_TRACING' in process.env) { diff --git a/dev/src/telemetry/disabled-trace-util.ts b/dev/src/telemetry/disabled-trace-util.ts index c4c6cce6a..bd703515a 100644 --- a/dev/src/telemetry/disabled-trace-util.ts +++ b/dev/src/telemetry/disabled-trace-util.ts @@ -16,6 +16,10 @@ import {Attributes, TraceUtil} from './trace-util'; import {Span} from './span'; +/** + * @private + * @internal + */ export class DisabledTraceUtil implements TraceUtil { // eslint-disable-next-line @typescript-eslint/no-unused-vars startSpan(name: string): Span { diff --git a/dev/src/telemetry/enabled-trace-util.ts b/dev/src/telemetry/enabled-trace-util.ts index 0fdbaf582..0ee4b043b 100644 --- a/dev/src/telemetry/enabled-trace-util.ts +++ b/dev/src/telemetry/enabled-trace-util.ts @@ -22,6 +22,7 @@ import { trace, Tracer, Span as OpenTelemetrySpan, + TracerProvider, } from '@opentelemetry/api'; import {Span} from './span'; @@ -33,22 +34,40 @@ import {DEFAULT_DATABASE_ID} from '../path'; import {DEFAULT_MAX_IDLE_CHANNELS} from '../index'; const serviceConfig = interfaces['google.firestore.v1.Firestore']; +/** + * @private + * @internal + */ export class EnabledTraceUtil implements TraceUtil { private tracer: Tracer; private settingsAttributes: Attributes; + // Visible for testing + tracerProvider: TracerProvider; + constructor(settings: Settings) { - let tracerProvider = settings.openTelemetryOptions?.tracerProvider; + let provider: TracerProvider | undefined = + settings.openTelemetryOptions?.tracerProvider; // If a TracerProvider has not been given to us, we try to use the global one. - if (!tracerProvider) { + if (!provider) { const {trace} = require('@opentelemetry/api'); - tracerProvider = trace.getTracerProvider(); + provider = trace.getTracerProvider(); } + // At this point provider is guaranteed to be defined because + // `trace.getTracerProvider()` does not return null or undefined. + this.tracerProvider = provider!; + const libVersion = require('../../../package.json').version; const libName = require('../../../package.json').name; - this.tracer = tracerProvider.getTracer(libName, libVersion); + try { + this.tracer = this.tracerProvider.getTracer(libName, libVersion); + } catch (e) { + throw new Error( + "the given value for 'tracerProvider' does not implement the TracerProvider interface." + ); + } this.settingsAttributes = {}; this.settingsAttributes['otel.scope.name'] = libName; diff --git a/dev/src/telemetry/span.ts b/dev/src/telemetry/span.ts index a4ad6e249..c4ed1ed0a 100644 --- a/dev/src/telemetry/span.ts +++ b/dev/src/telemetry/span.ts @@ -17,6 +17,10 @@ import {Span as OpenTelemetrySpan} from '@opentelemetry/api'; import {Attributes} from './trace-util'; +/** + * @private + * @internal + */ export class Span { constructor(private span?: OpenTelemetrySpan) {} diff --git a/dev/src/telemetry/trace-util.ts b/dev/src/telemetry/trace-util.ts index ff5a8b071..a38b614e1 100644 --- a/dev/src/telemetry/trace-util.ts +++ b/dev/src/telemetry/trace-util.ts @@ -16,9 +16,18 @@ import {Span} from './span'; +/** + * @private + * @internal + */ export interface Attributes { [attributeKey: string]: AttributeValue | undefined; } + +/** + * @private + * @internal + */ export declare type AttributeValue = | string | number @@ -67,6 +76,10 @@ export const ATTRIBUTE_KEY_TRANSACTION_TYPE = 'transaction_type'; export const ATTRIBUTE_KEY_ATTEMPTS_ALLOWED = 'attempts_allowed'; export const ATTRIBUTE_KEY_ATTEMPTS_REMAINING = 'attempts_remaining'; +/** + * @private + * @internal + */ export interface TraceUtil { startActiveSpan unknown>( name: string, diff --git a/dev/system-test/tracing.ts b/dev/system-test/tracing.ts index d808447ec..ee9fd6978 100644 --- a/dev/system-test/tracing.ts +++ b/dev/system-test/tracing.ts @@ -30,7 +30,7 @@ import { Context as OpenTelemetryContext, } from '@opentelemetry/api'; import {TraceExporter} from '@google-cloud/opentelemetry-cloud-trace-exporter'; -import {Settings} from '@google-cloud/firestore'; +import {FirestoreOpenTelemetryOptions, Settings} from '@google-cloud/firestore'; import { AlwaysOnSampler, BatchSpanProcessor, @@ -96,13 +96,6 @@ setLogFunction((msg: string) => { console.log(`LOG: ${msg}`); }); -// TODO(tracing): This should be moved to firestore.d.ts when we want to -// release the feature. -export interface FirestoreOpenTelemetryOptions { - enableTracing?: boolean; - tracerProvider?: any; -} - interface TestConfig { // In-Memory tests check trace correctness by inspecting traces in memory by // utilizing InMemorySpanExporter. These tests have `e2e` set to `false`. @@ -192,7 +185,6 @@ describe('Tracing Tests', () => { tracerProvider: TracerProvider ): FirestoreOpenTelemetryOptions { const options: FirestoreOpenTelemetryOptions = { - enableTracing: true, tracerProvider: undefined, }; @@ -920,6 +912,7 @@ describe('Tracing Tests', () => { await runFirestoreOperationInRootSpan(async () => { const query = firestore.collectionGroup('foo'); let numPartitions = 0; + // eslint-disable-next-line @typescript-eslint/no-unused-vars for await (const partition of query.getPartitions(3)) { numPartitions++; } diff --git a/dev/test/tracing.ts b/dev/test/tracing.ts index d356fc3fd..429afd07f 100644 --- a/dev/test/tracing.ts +++ b/dev/test/tracing.ts @@ -17,11 +17,15 @@ import {createInstance} from './util/helpers'; import {expect} from 'chai'; import {DisabledTraceUtil} from '../src/telemetry/disabled-trace-util'; import {EnabledTraceUtil} from '../src/telemetry/enabled-trace-util'; +import {NodeTracerProvider} from '@opentelemetry/sdk-trace-node'; +import {ProxyTracerProvider, trace} from '@opentelemetry/api'; describe('Firestore Tracing Controls', () => { let originalEnvVarValue: string | undefined; beforeEach(() => { + // Remove any prior global OpenTelemetry registrations. + trace.disable(); originalEnvVarValue = process.env.FIRESTORE_ENABLE_TRACING; }); @@ -33,116 +37,152 @@ describe('Firestore Tracing Controls', () => { } }); - it('default firestore settings have tracing disabled', async () => { + it('default firestore settings, no env var', async () => { const firestore = await createInstance(); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; - }); - - it('no openTelemetryOptions results in tracing disabled', async () => { - const firestore = await createInstance(undefined, { - openTelemetryOptions: undefined, - }); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; - }); - - it('openTelemetryOptions.enableTracing controls the tracing feature', async () => { - let firestore = await createInstance(undefined, { - openTelemetryOptions: { - enableTracing: undefined, - }, - }); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; - - firestore = await createInstance(undefined, { - openTelemetryOptions: { - enableTracing: false, - }, - }); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; - - firestore = await createInstance(undefined, { - openTelemetryOptions: { - enableTracing: true, - }, - }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; }); /// Tests to make sure environment variable can override settings. - it('env var disabled, default firestore settings', async () => { + it('default firestore settings, env var disabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; const firestore = await createInstance(); expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; }); - it('env var enabled, default firestore settings', async () => { + it('default firestore settings, env var enabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'ON'; const firestore = await createInstance(); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; }); - it('env var disabled, no openTelemetryOptions', async () => { + it('no openTelemetryOptions, no env var', async () => { + const firestore = await createInstance(undefined, { + openTelemetryOptions: undefined, + }); + expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; + + const firestore2 = await createInstance(undefined, { + openTelemetryOptions: { + tracerProvider: undefined, + }, + }); + expect(firestore2._traceUtil instanceof EnabledTraceUtil).to.be.true; + }); + + it('no openTelemetryOptions, env var disabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; const firestore = await createInstance(undefined, { openTelemetryOptions: undefined, }); expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; + + const firestore2 = await createInstance(undefined, { + openTelemetryOptions: { + tracerProvider: undefined, + }, + }); + expect(firestore2._traceUtil instanceof DisabledTraceUtil).to.be.true; }); - it('env var enabled, no openTelemetryOptions', async () => { + it('no openTelemetryOptions, env var enabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'ON'; const firestore = await createInstance(undefined, { openTelemetryOptions: undefined, }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; - }); - it('env var disabled, with openTelemetryOptions.enableTracing', async () => { - process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; - let firestore = await createInstance(undefined, { + const firestore2 = await createInstance(undefined, { openTelemetryOptions: { - enableTracing: undefined, + tracerProvider: undefined, }, }); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; + expect(firestore2._traceUtil instanceof EnabledTraceUtil).to.be.true; + }); - firestore = await createInstance(undefined, { + it('valid tracerProvider, no env var', async () => { + const firestore = await createInstance(undefined, { openTelemetryOptions: { - enableTracing: false, + tracerProvider: new NodeTracerProvider(), }, }); - expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; + expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; + }); - firestore = await createInstance(undefined, { + it('valid tracerProvider, env var disabled', async () => { + process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; + const firestore = await createInstance(undefined, { openTelemetryOptions: { - enableTracing: true, + tracerProvider: new NodeTracerProvider(), }, }); expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; }); - it('env var enabled, with openTelemetryOptions.enableTracing', async () => { + it('valid tracerProvider, env var enabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'ON'; - let firestore = await createInstance(undefined, { + const firestore = await createInstance(undefined, { openTelemetryOptions: { - enableTracing: undefined, + tracerProvider: new NodeTracerProvider(), }, }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; + }); + + it('uses the tracerProvider passed to it', async () => { + const myTracerProvider = new NodeTracerProvider(); - firestore = await createInstance(undefined, { + // Make another tracer provider the global tracer provider. + const globalTracerProvider = new NodeTracerProvider(); + globalTracerProvider.register(); + + const firestore = await createInstance(undefined, { openTelemetryOptions: { - enableTracing: false, + tracerProvider: myTracerProvider, }, }); + expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; + // Make sure the SDK uses the one that was given to it, not the global one. + expect( + (firestore._traceUtil as EnabledTraceUtil).tracerProvider === + myTracerProvider + ).to.be.true; + expect( + (firestore._traceUtil as EnabledTraceUtil).tracerProvider !== + globalTracerProvider + ).to.be.true; + }); + + it('uses the global tracerProvider if nothing was passed to it', async () => { + // Make another tracer provider the global tracer provider. + const globalTracerProvider = new NodeTracerProvider(); + globalTracerProvider.register(); + + const firestore = await createInstance(); - firestore = await createInstance(undefined, { - openTelemetryOptions: { - enableTracing: true, - }, - }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; + const enabledTraceUtil: EnabledTraceUtil = + firestore._traceUtil as EnabledTraceUtil; + // Since a TracerProvider is not provided to the SDK directly, the SDK obtains + // the tracer provider from the global `TraceAPI`. The `TraceAPI` returns a + // `ProxyTracerProvider` instance. To check equality, we need to compare our + // `globalTracerProvider` with the proxy's delegate. + const tracerProviderUsed = enabledTraceUtil.tracerProvider; + const actual = (tracerProviderUsed as ProxyTracerProvider).getDelegate(); + expect(actual === globalTracerProvider).to.be.true; + }); + + it('Generates an error if the given tracerProvider is not valid', async () => { + try { + await createInstance(undefined, { + openTelemetryOptions: {tracerProvider: 123}, + }); + } catch (e) { + expect( + e.toString() === + "the given value for 'tracerProvider' does not implement the TracerProvider interface" + ); + } }); }); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 51113bb1d..6e714fcf3 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -461,9 +461,33 @@ declare namespace FirebaseFirestore { */ preferRest?: boolean; + /** + * Settings related to telemetry collection by this client. + * @beta + */ + openTelemetryOptions?: FirestoreOpenTelemetryOptions; + [key: string]: any; // Accept other properties, such as GRPC settings. } + /** + * Options to configure telemetry collection. + * This is a 'beta' interface and may change in backwards incompatible ways. + * @beta + */ + export interface FirestoreOpenTelemetryOptions { + /** + * The SDK will use this OpenTelemetry TracerProvider to create spans. + * If not provided, the SDK will attempt to use the Global TracerProvider if + * one has been registered. In the absence of both, the SDK will not create + * trace spans. + * Even if a Global TracerProvider has been registered, one can still disable + * this client's span creation by passing in a "no-op" tracer provider here, + * or by setting the `FIRESTORE_ENABLE_TRACING=OFF` environment variable. + */ + tracerProvider?: any; + } + /** Options to configure a read-only transaction. */ export interface ReadOnlyTransactionOptions { /** Set to true to indicate a read-only transaction. */ From 3a373bb51aceacbe5e341f8766ea36f76b512130 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 22 Oct 2024 14:04:11 -0700 Subject: [PATCH 2/7] Better comment for the tracerProvider setting. --- types/firestore.d.ts | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 6e714fcf3..39a19da31 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -477,13 +477,12 @@ declare namespace FirebaseFirestore { */ export interface FirestoreOpenTelemetryOptions { /** - * The SDK will use this OpenTelemetry TracerProvider to create spans. - * If not provided, the SDK will attempt to use the Global TracerProvider if - * one has been registered. In the absence of both, the SDK will not create - * trace spans. - * Even if a Global TracerProvider has been registered, one can still disable - * this client's span creation by passing in a "no-op" tracer provider here, - * or by setting the `FIRESTORE_ENABLE_TRACING=OFF` environment variable. + * The OpenTelemetry TracerProvider instance that the SDK should use to + * create trace spans. If not provided, the SDK will use the Global TracerProvider. + * + * Even if a Global TracerProvider has been registered, users can still + * disable this client's span creation by passing in a "no-op" tracer provider + * here, or by setting the `FIRESTORE_ENABLE_TRACING=OFF` environment variable. */ tracerProvider?: any; } From 4b668440b43edb8a691f0c122ac69107b51824ee Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 29 Oct 2024 09:52:20 -0700 Subject: [PATCH 3/7] Address feedback. --- dev/src/telemetry/enabled-trace-util.ts | 2 +- dev/test/tracing.ts | 2 +- types/firestore.d.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dev/src/telemetry/enabled-trace-util.ts b/dev/src/telemetry/enabled-trace-util.ts index 0ee4b043b..7c8ca80f4 100644 --- a/dev/src/telemetry/enabled-trace-util.ts +++ b/dev/src/telemetry/enabled-trace-util.ts @@ -65,7 +65,7 @@ export class EnabledTraceUtil implements TraceUtil { this.tracer = this.tracerProvider.getTracer(libName, libVersion); } catch (e) { throw new Error( - "the given value for 'tracerProvider' does not implement the TracerProvider interface." + "The object provided for 'tracerProvider' does not conform to the TracerProvider interface." ); } diff --git a/dev/test/tracing.ts b/dev/test/tracing.ts index 429afd07f..8167f4f96 100644 --- a/dev/test/tracing.ts +++ b/dev/test/tracing.ts @@ -181,7 +181,7 @@ describe('Firestore Tracing Controls', () => { } catch (e) { expect( e.toString() === - "the given value for 'tracerProvider' does not implement the TracerProvider interface" + "The object provided for 'tracerProvider' does not conform to the TracerProvider interface." ); } }); diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 39a19da31..2c3c987da 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -482,7 +482,7 @@ declare namespace FirebaseFirestore { * * Even if a Global TracerProvider has been registered, users can still * disable this client's span creation by passing in a "no-op" tracer provider - * here, or by setting the `FIRESTORE_ENABLE_TRACING=OFF` environment variable. + * here, or by setting the `FIRESTORE_ENABLE_TRACING` environment variable to `OFF` or `FALSE`. */ tracerProvider?: any; } From 89e63b440ca0df44ccbf35017206591a2811595f Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 29 Oct 2024 12:38:20 -0700 Subject: [PATCH 4/7] Add an event for logical termination and add events to tests. --- dev/src/reference/query-util.ts | 7 ++++- dev/system-test/tracing.ts | 47 ++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/dev/src/reference/query-util.ts b/dev/src/reference/query-util.ts index 8aad95888..0ac50f775 100644 --- a/dev/src/reference/query-util.ts +++ b/dev/src/reference/query-util.ts @@ -180,6 +180,7 @@ export class QueryUtil< const tag = requestTag(); const startTime = Date.now(); const isExplain = explainOptions !== undefined; + const methodName = 'runQuery'; let numDocumentsReceived = 0; let lastReceivedDocument: QueryDocumentSnapshot< @@ -245,6 +246,11 @@ export class QueryUtil< if (proto.done) { logger('QueryUtil._stream', tag, 'Trigger Logical Termination.'); + this._firestore._traceUtil + .currentSpan() + .addEvent( + `Firestore.${methodName}: Received RunQueryResponse.Done.` + ); backendStream.unpipe(stream); backendStream.resume(); backendStream.end(); @@ -265,7 +271,6 @@ export class QueryUtil< let streamActive: Deferred; do { streamActive = new Deferred(); - const methodName = 'runQuery'; this._firestore._traceUtil .currentSpan() diff --git a/dev/system-test/tracing.ts b/dev/system-test/tracing.ts index ee9fd6978..c6255adbd 100644 --- a/dev/system-test/tracing.ts +++ b/dev/system-test/tracing.ts @@ -652,7 +652,7 @@ describe('Tracing Tests', () => { // Expect that the span exists first. const span = getSpanByName(spanName); - expect(span).to.not.be.null; + expect(span, `Could not find the span named ${spanName}`).to.not.be.null; // Assert that the expected attributes are present in the span attributes. // Note that the span attributes may be a superset of the attributes passed @@ -664,6 +664,29 @@ describe('Tracing Tests', () => { } } + // Ensures that the given span exists and has the given attributes. + function expectSpanHasEvents(spanName: string, eventNames: string[]): void { + // The Cloud Trace API does not return span attributes and events. + if (testConfig.e2e) { + return; + } + + // Expect that the span exists first. + const span = getSpanByName(spanName); + expect(span, `Could not find the span named ${spanName}`).to.not.be.null; + + // Assert that the expected attributes are present in the span attributes. + // Note that the span attributes may be a superset of the attributes passed + // to this function. + if (span?.events) { + const numEvents = eventNames.length; + expect(numEvents).to.equal(span.events.length); + for (let i = 0; i < numEvents; ++i) { + expect(span.events[i].name).to.equal(eventNames[i]); + } + } + } + describe(IN_MEMORY_TEST_SUITE_TITLE, () => { describe(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { describe(GRPC_TEST_SUITE_TITLE, () => { @@ -745,6 +768,11 @@ describe('Tracing Tests', () => { SPAN_NAME_DOC_REF_GET, SPAN_NAME_BATCH_GET_DOCUMENTS ); + expectSpanHasEvents(SPAN_NAME_BATCH_GET_DOCUMENTS, [ + 'Firestore.batchGetDocuments: Start', + 'Firestore.batchGetDocuments: First response received', + 'Firestore.batchGetDocuments: Completed', + ]); }); it('document reference create()', async () => { @@ -812,6 +840,11 @@ describe('Tracing Tests', () => { ); await waitForCompletedSpans(2); expectSpanHierarchy(SPAN_NAME_TEST_ROOT, SPAN_NAME_AGGREGATION_QUERY_GET); + expectSpanHasEvents(SPAN_NAME_AGGREGATION_QUERY_GET, [ + 'Firestore.runAggregationQuery: Start', + 'Firestore.runAggregationQuery: First response received', + 'Firestore.runAggregationQuery: Completed', + ]); }); it('collection reference add()', async () => { @@ -844,6 +877,13 @@ describe('Tracing Tests', () => { ); await waitForCompletedSpans(2); expectSpanHierarchy(SPAN_NAME_TEST_ROOT, SPAN_NAME_QUERY_GET); + expectSpanHasEvents(SPAN_NAME_QUERY_GET, [ + 'RunQuery', + 'Firestore.runQuery: Start', + 'Firestore.runQuery: First response received', + 'Firestore.runQuery: Received RunQueryResponse.Done.', + 'Firestore.runQuery: Completed', + ]); }); it('firestore getAll()', async () => { @@ -854,6 +894,11 @@ describe('Tracing Tests', () => { ); await waitForCompletedSpans(2); expectSpanHierarchy(SPAN_NAME_TEST_ROOT, SPAN_NAME_BATCH_GET_DOCUMENTS); + expectSpanHasEvents(SPAN_NAME_BATCH_GET_DOCUMENTS, [ + 'Firestore.batchGetDocuments: Start', + 'Firestore.batchGetDocuments: First response received', + 'Firestore.batchGetDocuments: Completed', + ]); }); it('transaction', async () => { From 7663246c6439e506d9b4bec6e320f0af30c0a7ae Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 26 Nov 2024 09:44:11 -0800 Subject: [PATCH 5/7] Address API naming feedback. --- dev/src/telemetry/enabled-trace-util.ts | 2 +- dev/system-test/tracing.ts | 2 +- dev/test/tracing.ts | 28 ++++++++++++------------- types/firestore.d.ts | 2 +- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/dev/src/telemetry/enabled-trace-util.ts b/dev/src/telemetry/enabled-trace-util.ts index 7c8ca80f4..9b78ee99a 100644 --- a/dev/src/telemetry/enabled-trace-util.ts +++ b/dev/src/telemetry/enabled-trace-util.ts @@ -47,7 +47,7 @@ export class EnabledTraceUtil implements TraceUtil { constructor(settings: Settings) { let provider: TracerProvider | undefined = - settings.openTelemetryOptions?.tracerProvider; + settings.openTelemetry?.tracerProvider; // If a TracerProvider has not been given to us, we try to use the global one. if (!provider) { diff --git a/dev/system-test/tracing.ts b/dev/system-test/tracing.ts index c6255adbd..59adc9dbd 100644 --- a/dev/system-test/tracing.ts +++ b/dev/system-test/tracing.ts @@ -277,7 +277,7 @@ describe('Tracing Tests', () => { const settings: Settings = { preferRest: testConfig.preferRest, - openTelemetryOptions: getOpenTelemetryOptions(tracerProvider), + openTelemetry: getOpenTelemetryOptions(tracerProvider), }; // Named-database tests use an environment variable to specify the database ID. Add it to the settings. diff --git a/dev/test/tracing.ts b/dev/test/tracing.ts index 8167f4f96..16104951a 100644 --- a/dev/test/tracing.ts +++ b/dev/test/tracing.ts @@ -56,44 +56,44 @@ describe('Firestore Tracing Controls', () => { expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; }); - it('no openTelemetryOptions, no env var', async () => { + it('no openTelemetry settings, no env var', async () => { const firestore = await createInstance(undefined, { - openTelemetryOptions: undefined, + openTelemetry: undefined, }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; const firestore2 = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: undefined, }, }); expect(firestore2._traceUtil instanceof EnabledTraceUtil).to.be.true; }); - it('no openTelemetryOptions, env var disabled', async () => { + it('no openTelemetry settings, env var disabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; const firestore = await createInstance(undefined, { - openTelemetryOptions: undefined, + openTelemetry: undefined, }); expect(firestore._traceUtil instanceof DisabledTraceUtil).to.be.true; const firestore2 = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: undefined, }, }); expect(firestore2._traceUtil instanceof DisabledTraceUtil).to.be.true; }); - it('no openTelemetryOptions, env var enabled', async () => { + it('no openTelemetry settings, env var enabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'ON'; const firestore = await createInstance(undefined, { - openTelemetryOptions: undefined, + openTelemetry: undefined, }); expect(firestore._traceUtil instanceof EnabledTraceUtil).to.be.true; const firestore2 = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: undefined, }, }); @@ -102,7 +102,7 @@ describe('Firestore Tracing Controls', () => { it('valid tracerProvider, no env var', async () => { const firestore = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: new NodeTracerProvider(), }, }); @@ -112,7 +112,7 @@ describe('Firestore Tracing Controls', () => { it('valid tracerProvider, env var disabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'OFF'; const firestore = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: new NodeTracerProvider(), }, }); @@ -122,7 +122,7 @@ describe('Firestore Tracing Controls', () => { it('valid tracerProvider, env var enabled', async () => { process.env.FIRESTORE_ENABLE_TRACING = 'ON'; const firestore = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: new NodeTracerProvider(), }, }); @@ -137,7 +137,7 @@ describe('Firestore Tracing Controls', () => { globalTracerProvider.register(); const firestore = await createInstance(undefined, { - openTelemetryOptions: { + openTelemetry: { tracerProvider: myTracerProvider, }, }); @@ -176,7 +176,7 @@ describe('Firestore Tracing Controls', () => { it('Generates an error if the given tracerProvider is not valid', async () => { try { await createInstance(undefined, { - openTelemetryOptions: {tracerProvider: 123}, + openTelemetry: {tracerProvider: 123}, }); } catch (e) { expect( diff --git a/types/firestore.d.ts b/types/firestore.d.ts index 2c3c987da..cfff47c98 100644 --- a/types/firestore.d.ts +++ b/types/firestore.d.ts @@ -465,7 +465,7 @@ declare namespace FirebaseFirestore { * Settings related to telemetry collection by this client. * @beta */ - openTelemetryOptions?: FirestoreOpenTelemetryOptions; + openTelemetry?: FirestoreOpenTelemetryOptions; [key: string]: any; // Accept other properties, such as GRPC settings. } From de36911767092248c53d5c1c3e025470feb3fc07 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 26 Nov 2024 14:27:02 -0800 Subject: [PATCH 6/7] testing the github actions --- dev/system-test/tracing.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/dev/system-test/tracing.ts b/dev/system-test/tracing.ts index 59adc9dbd..3f3046ed1 100644 --- a/dev/system-test/tracing.ts +++ b/dev/system-test/tracing.ts @@ -153,7 +153,7 @@ class SpanData { } } -describe('Tracing Tests', () => { +describe.only('Tracing Tests', () => { let firestore: Firestore; let tracerProvider: NodeTracerProvider; let inMemorySpanExporter: InMemorySpanExporter; @@ -680,23 +680,24 @@ describe('Tracing Tests', () => { // to this function. if (span?.events) { const numEvents = eventNames.length; - expect(numEvents).to.equal(span.events.length); + console.log(`${span.events.map(value => value.name)} did not match ${eventNames}`); + expect(numEvents).to.equal(span.events.length, `${span.events.map(value => value.name)} did not match ${eventNames}`); for (let i = 0; i < numEvents; ++i) { expect(span.events[i].name).to.equal(eventNames[i]); } } } - describe(IN_MEMORY_TEST_SUITE_TITLE, () => { - describe(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { - describe(GRPC_TEST_SUITE_TITLE, () => { + describe.only(IN_MEMORY_TEST_SUITE_TITLE, () => { + describe.only(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { + describe.only(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); }); runTestCases(); afterEach(async () => afterEachTest()); }); - describe(REST_TEST_SUITE_TITLE, () => { + describe.skip(REST_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); }); @@ -704,7 +705,7 @@ describe('Tracing Tests', () => { afterEach(async () => afterEachTest()); }); }); - describe(GLOBAL_OTEL_TEST_SUITE_TITLE, () => { + describe.skip(GLOBAL_OTEL_TEST_SUITE_TITLE, () => { describe(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); @@ -722,7 +723,7 @@ describe('Tracing Tests', () => { }); }); - describe(E2E_TEST_SUITE_TITLE, () => { + describe.skip(E2E_TEST_SUITE_TITLE, () => { describe(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { describe(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { @@ -871,7 +872,7 @@ describe('Tracing Tests', () => { ); }); - it('query get()', async () => { + it.only('query get()', async () => { await runFirestoreOperationInRootSpan(() => firestore.collection('foo').where('foo', '==', 'bar').limit(1).get() ); From 4f46b08074decfe9d7e67a8f41cf45ec3e633722 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 26 Nov 2024 14:38:33 -0800 Subject: [PATCH 7/7] remove event not in prod. --- dev/system-test/tracing.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/dev/system-test/tracing.ts b/dev/system-test/tracing.ts index 3f3046ed1..4e19ccf37 100644 --- a/dev/system-test/tracing.ts +++ b/dev/system-test/tracing.ts @@ -680,7 +680,6 @@ describe.only('Tracing Tests', () => { // to this function. if (span?.events) { const numEvents = eventNames.length; - console.log(`${span.events.map(value => value.name)} did not match ${eventNames}`); expect(numEvents).to.equal(span.events.length, `${span.events.map(value => value.name)} did not match ${eventNames}`); for (let i = 0; i < numEvents; ++i) { expect(span.events[i].name).to.equal(eventNames[i]); @@ -688,16 +687,16 @@ describe.only('Tracing Tests', () => { } } - describe.only(IN_MEMORY_TEST_SUITE_TITLE, () => { - describe.only(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { - describe.only(GRPC_TEST_SUITE_TITLE, () => { + describe(IN_MEMORY_TEST_SUITE_TITLE, () => { + describe(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { + describe(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); }); runTestCases(); afterEach(async () => afterEachTest()); }); - describe.skip(REST_TEST_SUITE_TITLE, () => { + describe(REST_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); }); @@ -705,7 +704,7 @@ describe.only('Tracing Tests', () => { afterEach(async () => afterEachTest()); }); }); - describe.skip(GLOBAL_OTEL_TEST_SUITE_TITLE, () => { + describe(GLOBAL_OTEL_TEST_SUITE_TITLE, () => { describe(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { beforeEachTest(this.currentTest!); @@ -723,7 +722,7 @@ describe.only('Tracing Tests', () => { }); }); - describe.skip(E2E_TEST_SUITE_TITLE, () => { + describe(E2E_TEST_SUITE_TITLE, () => { describe(NON_GLOBAL_OTEL_TEST_SUITE_TITLE, () => { describe(GRPC_TEST_SUITE_TITLE, () => { beforeEach(function () { @@ -872,7 +871,7 @@ describe.only('Tracing Tests', () => { ); }); - it.only('query get()', async () => { + it('query get()', async () => { await runFirestoreOperationInRootSpan(() => firestore.collection('foo').where('foo', '==', 'bar').limit(1).get() ); @@ -882,7 +881,6 @@ describe.only('Tracing Tests', () => { 'RunQuery', 'Firestore.runQuery: Start', 'Firestore.runQuery: First response received', - 'Firestore.runQuery: Received RunQueryResponse.Done.', 'Firestore.runQuery: Completed', ]); });