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

feat(otel): Handle inconsistent instrumenter option #6153

Merged
merged 2 commits into from
Nov 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions packages/opentelemetry-node/src/spanprocessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
if (sentryParentSpan) {
const sentryChildSpan = sentryParentSpan.startChild({
description: otelSpan.name,
// instrumentor: 'otel',
instrumenter: 'otel',
startTimestamp: convertOtelTimeToSeconds(otelSpan.startTime),
spanId: otelSpanId,
});
Expand All @@ -56,7 +56,7 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
const transaction = hub.startTransaction({
name: otelSpan.name,
...traceCtx,
// instrumentor: 'otel',
instrumenter: 'otel',
startTimestamp: convertOtelTimeToSeconds(otelSpan.startTime),
spanId: otelSpanId,
});
Expand Down
13 changes: 13 additions & 0 deletions packages/tracing/src/hubextensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,19 @@ function _startTransaction(
const client = this.getClient();
const options: Partial<ClientOptions> = (client && client.getOptions()) || {};

const configInstrumenter = options.instrumenter || 'sentry';
const transactionInstrumenter = transactionContext.instrumenter || 'sentry';

if (configInstrumenter !== transactionInstrumenter) {
__DEBUG_BUILD__ &&
logger.error(
`A transaction was started with instrumenter=\`${transactionInstrumenter}\`, but the SDK is configured with the \`${configInstrumenter}\` instrumenter.
The transaction will not be sampled. Please use the ${configInstrumenter} instrumentation to start transactions.`,
);

transactionContext.sampled = false;
}

let transaction = new Transaction(transactionContext, this);
transaction = sample(transaction, options, {
parentSampled: transactionContext.parentSampled,
Expand Down
22 changes: 22 additions & 0 deletions packages/tracing/test/hub.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const mathRandom = jest.spyOn(Math, 'random');
jest.spyOn(Transaction.prototype, 'setMetadata');
jest.spyOn(logger, 'warn');
jest.spyOn(logger, 'log');
jest.spyOn(logger, 'error');
jest.spyOn(utilsModule, 'isNodeEnv');

addDOMPropertiesToGlobal(['XMLHttpRequest', 'Event', 'location', 'document']);
Expand Down Expand Up @@ -377,6 +378,27 @@ describe('Hub', () => {
expect(client.captureEvent).not.toBeCalled();
});

it('should drop transactions when using wrong instrumenter', () => {
const options = getDefaultBrowserClientOptions({ tracesSampleRate: 1, instrumenter: 'otel' });
const client = new BrowserClient(options);
jest.spyOn(client, 'captureEvent');

const hub = new Hub(client);
makeMain(hub);
const transaction = hub.startTransaction({ name: 'dogpark' });

jest.spyOn(transaction, 'finish');
transaction.finish();

expect(transaction.sampled).toBe(false);
expect(transaction.finish).toReturnWith(undefined);
expect(client.captureEvent).not.toBeCalled();
expect(logger.error).toHaveBeenCalledWith(
`A transaction was started with instrumenter=\`sentry\`, but the SDK is configured with the \`otel\` instrumenter.
The transaction will not be sampled. Please use the otel instrumentation to start transactions.`,
);
});

describe('sampling inheritance', () => {
it('should propagate sampling decision to child spans', () => {
const options = getDefaultBrowserClientOptions({ tracesSampleRate: Math.random() });
Expand Down