Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Add settings attributes to trace spans. #2181

Merged
merged 3 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions dev/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ export const DEFAULT_MAX_TRANSACTION_ATTEMPTS = 5;
/*!
* The default number of idle GRPC channel to keep.
*/
const DEFAULT_MAX_IDLE_CHANNELS = 1;
export const DEFAULT_MAX_IDLE_CHANNELS = 1;

/*!
* The maximum number of concurrent requests supported by a single GRPC channel,
Expand Down Expand Up @@ -588,7 +588,6 @@ export class Firestore implements firestore.Firestore {
}

this.validateAndApplySettings({...settings, ...libraryHeader});

this._traceUtil = this.newTraceUtilInstance(this._settings);

const retryConfig = serviceConfig.retry_params.default;
Expand Down Expand Up @@ -695,6 +694,7 @@ export class Firestore implements firestore.Firestore {

const mergedSettings = {...this._settings, ...settings};
this.validateAndApplySettings(mergedSettings);
this._traceUtil = this.newTraceUtilInstance(this._settings);
this._settingsFrozen = true;
}

Expand Down Expand Up @@ -788,7 +788,6 @@ export class Firestore implements firestore.Firestore {
return temp;
};
this._serializer = new Serializer(this);
this._traceUtil = this.newTraceUtilInstance(this._settings);
}

private newTraceUtilInstance(settings: firestore.Settings): TraceUtil {
Expand All @@ -808,10 +807,6 @@ export class Firestore implements firestore.Firestore {
}

if (createEnabledInstance) {
// Re-use the existing EnabledTraceUtil if one has been created.
if (this._traceUtil && this._traceUtil instanceof EnabledTraceUtil) {
return this._traceUtil;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are now capturing settings as part of the trace attributes, if the settings change we need to also change the traceUtil instance. So we should not re-use the existing one.

return new EnabledTraceUtil(settings);
} else {
return new DisabledTraceUtil();
Expand Down Expand Up @@ -1549,6 +1544,10 @@ export class Firestore implements firestore.Firestore {
'Detected project ID: %s',
this._projectId
);

// If the project ID was undefined when the TraceUtil was set up, we
// need to record it.
this._traceUtil.recordProjectId(this.projectId);
} catch (err) {
logger(
'Firestore.initializeIfNeeded',
Expand Down
3 changes: 3 additions & 0 deletions dev/src/telemetry/disabled-trace-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,7 @@ export class DisabledTraceUtil implements TraceUtil {
currentSpan(): Span {
return new Span();
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
recordProjectId(projectId: string): void {}
}
98 changes: 96 additions & 2 deletions dev/src/telemetry/enabled-trace-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,17 @@ import {
} from '@opentelemetry/api';

import {Span} from './span';
import {Attributes, TraceUtil} from './trace-util';
import {ATTRIBUTE_SETTINGS_PREFIX, Attributes, TraceUtil} from './trace-util';

import {interfaces} from '../v1/firestore_client_config.json';
import {FirestoreClient} from '../v1';
import {DEFAULT_DATABASE_ID} from '../path';
import {DEFAULT_MAX_IDLE_CHANNELS} from '../index';
const serviceConfig = interfaces['google.firestore.v1.Firestore'];

export class EnabledTraceUtil implements TraceUtil {
private tracer: Tracer;
private settingsAttributes: Attributes;

constructor(settings: Settings) {
let tracerProvider = settings.openTelemetryOptions?.tracerProvider;
Expand All @@ -42,6 +49,85 @@ export class EnabledTraceUtil implements TraceUtil {
const libVersion = require('../../../package.json').version;
const libName = require('../../../package.json').name;
this.tracer = tracerProvider.getTracer(libName, libVersion);

this.settingsAttributes = {};
this.settingsAttributes['otel.scope.name'] = libName;
this.settingsAttributes['otel.scope.version'] = libVersion;

if (settings.projectId) {
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.project_id`] =
settings.projectId;
}

this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.database_id`] =
settings.databaseId || DEFAULT_DATABASE_ID;

const host =
settings.servicePath ?? settings.host ?? 'firestore.googleapis.com';
const port = settings.port ?? FirestoreClient.port;
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.host`] =
`${host}:${port}`;

if (settings.preferRest !== undefined) {
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.prefer_REST`] =
settings.preferRest;
}

this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_idle_channels`] =
settings.maxIdleChannels ?? DEFAULT_MAX_IDLE_CHANNELS;

const defaultRetrySettings = serviceConfig.retry_params.default;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why there are retry and timeout configuration? how this is related to otel tracing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is adding trace attributes to the spans. The purpose is that if a user has misconfigured their SDK, their configurations will surface in the trace and may provide insights into the issues they're running into. This is similar to the Java impl. Although not all java configurations exist in the node.js SDK.

const customRetrySettings =
settings.clientConfig?.interfaces?.['google.firestore.v1.Firestore']?.[
'retry_params'
]?.['default'];
this.settingsAttributes[
`${ATTRIBUTE_SETTINGS_PREFIX}.initial_retry_delay`
] = this.millisToSecondString(
customRetrySettings?.initial_retry_delay_millis ??
defaultRetrySettings.initial_retry_delay_millis
);
this.settingsAttributes[
`${ATTRIBUTE_SETTINGS_PREFIX}.initial_rpc_timeout`
] = this.millisToSecondString(
customRetrySettings?.initial_rpc_timeout_millis ??
defaultRetrySettings.initial_rpc_timeout_millis
);
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.total_timeout`] =
this.millisToSecondString(
customRetrySettings?.total_timeout_millis ??
defaultRetrySettings.total_timeout_millis
);
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_retry_delay`] =
this.millisToSecondString(
customRetrySettings?.max_retry_delay_millis ??
defaultRetrySettings.max_retry_delay_millis
);
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_rpc_timeout`] =
this.millisToSecondString(
customRetrySettings?.max_rpc_timeout_millis ??
defaultRetrySettings.max_rpc_timeout_millis
);
this.settingsAttributes[
`${ATTRIBUTE_SETTINGS_PREFIX}.retry_delay_multiplier`
] =
customRetrySettings?.retry_delay_multiplier.toString() ??
defaultRetrySettings.retry_delay_multiplier.toString();
this.settingsAttributes[
`${ATTRIBUTE_SETTINGS_PREFIX}.rpc_timeout_multiplier`
] =
customRetrySettings?.rpc_timeout_multiplier.toString() ??
defaultRetrySettings.rpc_timeout_multiplier.toString();
}

recordProjectId(projectId: string): void {
this.settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.project_id`] =
projectId;
this.currentSpan().setAttributes(this.settingsAttributes);
}

private millisToSecondString(millis: number): string {
return `${millis / 1000}s`;
}

private endSpan(otelSpan: OpenTelemetrySpan, error: Error): void {
Expand All @@ -64,6 +150,8 @@ export class EnabledTraceUtil implements TraceUtil {
attributes: attributes,
},
(otelSpan: OpenTelemetrySpan) => {
this.addCommonAttributes(otelSpan);

// Note that if `fn` returns a `Promise`, we want the otelSpan to end
// after the `Promise` has resolved, NOT after the `fn` has returned.
// Therefore, we should not use a `finally` clause to end the otelSpan.
Expand Down Expand Up @@ -94,10 +182,16 @@ export class EnabledTraceUtil implements TraceUtil {
}

startSpan(name: string): Span {
return new Span(this.tracer.startSpan(name, undefined, context.active()));
const otelSpan = this.tracer.startSpan(name, undefined, context.active());
this.addCommonAttributes(otelSpan);
return new Span(otelSpan);
}

currentSpan(): Span {
return new Span(trace.getActiveSpan());
}

addCommonAttributes(otelSpan: OpenTelemetrySpan): void {
otelSpan.setAttributes(this.settingsAttributes);
}
}
4 changes: 4 additions & 0 deletions dev/src/telemetry/trace-util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ export const SPAN_NAME_TRANSACTION_COMMIT = 'Transaction.Commit';
export const SPAN_NAME_BATCH_COMMIT = 'Batch.Commit';
export const SPAN_NAME_PARTITION_QUERY = 'PartitionQuery';
export const SPAN_NAME_BULK_WRITER_COMMIT = 'BulkWriter.Commit';
export const ATTRIBUTE_SERVICE_PREFIX = 'gcp.firestore';
export const ATTRIBUTE_SETTINGS_PREFIX = `${ATTRIBUTE_SERVICE_PREFIX}.settings`;
export const ATTRIBUTE_KEY_DOC_COUNT = 'doc_count';
export const ATTRIBUTE_KEY_IS_TRANSACTIONAL = 'transactional';
export const ATTRIBUTE_KEY_NUM_RESPONSES = 'response_count';
Expand All @@ -74,4 +76,6 @@ export interface TraceUtil {
startSpan(name: string): Span;

currentSpan(): Span;

recordProjectId(projectId: string): void;
}
72 changes: 67 additions & 5 deletions dev/system-test/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import {verifyInstance} from '../test/util/helpers';
import {
ATTRIBUTE_KEY_DOC_COUNT,
ATTRIBUTE_SETTINGS_PREFIX,
SPAN_NAME_AGGREGATION_QUERY_GET,
SPAN_NAME_BATCH_COMMIT,
SPAN_NAME_BATCH_GET_DOCUMENTS,
Expand All @@ -66,7 +67,6 @@
SPAN_NAME_TRANSACTION_RUN,
} from '../src/telemetry/trace-util';
import {AsyncLocalStorageContextManager} from '@opentelemetry/context-async-hooks';
import {deepStrictEqual} from 'assert';
import {cloudtrace_v1, auth as gAuth} from '@googleapis/cloudtrace';
import Schema$Trace = cloudtrace_v1.Schema$Trace;
import Schema$TraceSpan = cloudtrace_v1.Schema$TraceSpan;
Expand Down Expand Up @@ -100,7 +100,7 @@
// release the feature.
export interface FirestoreOpenTelemetryOptions {
enableTracing?: boolean;
tracerProvider?: any;

Check warning on line 103 in dev/system-test/tracing.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected any. Specify a different type
}

interface TestConfig {
Expand Down Expand Up @@ -306,6 +306,43 @@
firestore = new Firestore(settings);
}

function getSettingsAttributes(): Attributes {
const settingsAttributes: Attributes = {};
settingsAttributes['otel.scope.name'] = require('../../package.json').name;
settingsAttributes['otel.scope.version'] =
require('../../package.json').version;
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.database_id`] =
firestore.databaseId;
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.host`] =
'firestore.googleapis.com:443';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.prefer_REST`] =
testConfig.preferRest;
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_idle_channels`] = 1;
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.initial_retry_delay`] =
'0.1s';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.initial_rpc_timeout`] =
'60s';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.total_timeout`] = '600s';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_retry_delay`] = '60s';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.max_rpc_timeout`] = '60s';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.retry_delay_multiplier`] =
'1.3';
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.rpc_timeout_multiplier`] =
'1';

// Project ID is not set on the Firestore object until _after_ the first
// operation is done. Therefore, the spans that are created _before_ the
// first operation do not contain a project ID.
try {
const projectId = firestore.projectId;
settingsAttributes[`${ATTRIBUTE_SETTINGS_PREFIX}.project_id`] = projectId;
} catch (e) {
// Project ID has not been set yet.
}

return settingsAttributes;
}

// Take a function and runs it inside a new root span. This makes it possible to
// encapsulate all the SDK-generated spans inside a test root span. It also makes
// it easy to query a trace storage backend for a known trace ID and span Id.
Expand Down Expand Up @@ -588,7 +625,26 @@
parentSpan.traceId,
`'${childSpan.name}' and '${parentSpan.name}' spans do not belong to the same trace`
);
// TODO(tracing): expect that each span has the needed attributes.

// The Cloud Trace API does not return span attributes and events.
if (!testConfig.e2e) {
const settingsAttributes = getSettingsAttributes();
for (const attributesKey in settingsAttributes) {
if (
attributesKey.endsWith('.project_id') &&
i + 1 !== matchingSpanHierarchy.length
) {
// Project ID is not set on the Firestore object until _after_ the first
// operation is done. Therefore, the spans that are created _before_ the
// first operation do not contain a project ID. So, we'll just compare
// this attribute on the leaf spans.
} else {
expect(childSpan.attributes[attributesKey]).to.be.equal(
settingsAttributes[attributesKey]
);
}
}
}
}
}

Expand All @@ -597,7 +653,7 @@
spanName: string,
attributes: Attributes
): void {
// TODO(tracing): The current Cloud Trace API does not return span attributes and events.
// The Cloud Trace API does not return span attributes and events.
if (testConfig.e2e) {
return;
}
Expand All @@ -606,8 +662,14 @@
const span = getSpanByName(spanName);
expect(span).to.not.be.null;

// Assert that the attributes are the same.
deepStrictEqual(span!.attributes, attributes);
// 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.
for (const attributesKey in attributes) {
expect(span!.attributes[attributesKey]).to.be.equal(
attributes[attributesKey]
);
}
}

describe(IN_MEMORY_TEST_SUITE_TITLE, () => {
Expand Down Expand Up @@ -858,7 +920,7 @@
await runFirestoreOperationInRootSpan(async () => {
const query = firestore.collectionGroup('foo');
let numPartitions = 0;
for await (const partition of query.getPartitions(3)) {

Check warning on line 923 in dev/system-test/tracing.ts

View workflow job for this annotation

GitHub Actions / lint

'partition' is assigned a value but never used
numPartitions++;
}
return numPartitions;
Expand Down
Loading