Skip to content

Commit

Permalink
fix(node): Improve OTEL validation logic (#13079)
Browse files Browse the repository at this point in the history
When using the SDK without tracing, the span processor is not required -
we should reflect this in our validation logic.

While at it, I also improved the warning for not using the
SentrySampler.
  • Loading branch information
mydea authored Jul 29, 2024
1 parent df45d65 commit 9a8e910
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 3 deletions.
9 changes: 7 additions & 2 deletions packages/node/src/sdk/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,12 @@ export function validateOpenTelemetrySetup(): void {

const setup = openTelemetrySetupCheck();

const required = ['SentrySpanProcessor', 'SentryContextManager', 'SentryPropagator'] as const;
const required: ReturnType<typeof openTelemetrySetupCheck> = ['SentryContextManager', 'SentryPropagator'];

if (hasTracingEnabled()) {
required.push('SentrySpanProcessor');
}

for (const k of required) {
if (!setup.includes(k)) {
logger.error(
Expand All @@ -206,7 +211,7 @@ export function validateOpenTelemetrySetup(): void {

if (!setup.includes('SentrySampler')) {
logger.warn(
'You have to set up the SentrySampler. Without this, the OpenTelemetry & Sentry integration may still work, but sample rates set for the Sentry SDK will not be respected.',
'You have to set up the SentrySampler. Without this, the OpenTelemetry & Sentry integration may still work, but sample rates set for the Sentry SDK will not be respected. If you use a custom sampler, make sure to use `wrapSamplingDecision`.',
);
}
}
Expand Down
66 changes: 65 additions & 1 deletion packages/node/test/sdk/init.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import type { Integration } from '@sentry/types';
import { logger } from '@sentry/utils';

import * as SentryOpentelemetry from '@sentry/opentelemetry';
import { getClient, getIsolationScope } from '../../src/';
import * as auto from '../../src/integrations/tracing';
import { init } from '../../src/sdk';
import { init, validateOpenTelemetrySetup } from '../../src/sdk';
import { NodeClient } from '../../src/sdk/client';
import { cleanupOtel } from '../helpers/mockSdkInit';

Expand Down Expand Up @@ -193,3 +195,65 @@ describe('init()', () => {
});
});
});

describe('validateOpenTelemetrySetup', () => {
afterEach(() => {
global.__SENTRY__ = {};
cleanupOtel();
jest.clearAllMocks();
});

it('works with correct setup', () => {
const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});

jest.spyOn(SentryOpentelemetry, 'openTelemetrySetupCheck').mockImplementation(() => {
return ['SentryContextManager', 'SentryPropagator', 'SentrySampler'];
});

validateOpenTelemetrySetup();

expect(errorSpy).toHaveBeenCalledTimes(0);
expect(warnSpy).toHaveBeenCalledTimes(0);
});

it('works with missing setup, without tracing', () => {
const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});

jest.spyOn(SentryOpentelemetry, 'openTelemetrySetupCheck').mockImplementation(() => {
return [];
});

validateOpenTelemetrySetup();

// Without tracing, this is expected only twice
expect(errorSpy).toHaveBeenCalledTimes(2);
expect(warnSpy).toHaveBeenCalledTimes(1);

expect(errorSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentryContextManager.'));
expect(errorSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentryPropagator.'));
expect(warnSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentrySampler.'));
});

it('works with missing setup, with tracing', () => {
const errorSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
const warnSpy = jest.spyOn(logger, 'warn').mockImplementation(() => {});

jest.spyOn(SentryOpentelemetry, 'openTelemetrySetupCheck').mockImplementation(() => {
return [];
});

init({ dsn: PUBLIC_DSN, skipOpenTelemetrySetup: true, tracesSampleRate: 1 });

validateOpenTelemetrySetup();

expect(errorSpy).toHaveBeenCalledTimes(3);
expect(warnSpy).toHaveBeenCalledTimes(1);

expect(errorSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentryContextManager.'));
expect(errorSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentryPropagator.'));
expect(errorSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentrySpanProcessor.'));
expect(warnSpy).toBeCalledWith(expect.stringContaining('You have to set up the SentrySampler.'));
});
});

0 comments on commit 9a8e910

Please sign in to comment.