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(v8/core): Remove span.instrumenter and instrumenter option #10769

Merged
merged 2 commits into from
Feb 23, 2024
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
1 change: 0 additions & 1 deletion packages/bun/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export function getDefaultBunClientOptions(options: Partial<BunClientOptions> =
integrations: [],
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})),
stackParser: () => [],
instrumenter: 'sentry',
...options,
};
}
15 changes: 0 additions & 15 deletions packages/core/src/tracing/hubextensions.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import type { ClientOptions, CustomSamplingContext, Hub, TransactionContext } from '@sentry/types';
import { logger } from '@sentry/utils';
import { getMainCarrier } from '../asyncContext';

import { DEBUG_BUILD } from '../debug-build';
import { registerErrorInstrumentation } from './errors';
import { IdleTransaction } from './idletransaction';
import { sampleTransaction } from './sampling';
Expand Down Expand Up @@ -32,19 +30,6 @@ 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;
}

// eslint-disable-next-line deprecation/deprecation
let transaction = new Transaction(transactionContext, this);
transaction = sampleTransaction(transaction, options, {
Expand Down
15 changes: 0 additions & 15 deletions packages/core/src/tracing/sentrySpan.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type {
Instrumenter,
Primitive,
Span as SpanInterface,
SpanAttributeValue,
Expand Down Expand Up @@ -90,18 +89,6 @@ export class SentrySpan implements SpanInterface {
* @deprecated Use top level `Sentry.getRootSpan()` instead
*/
public transaction?: Transaction;

/**
* The instrumenter that created this span.
*
* TODO (v8): This can probably be replaced by an `instanceOf` check of the span class.
* the instrumenter can only be sentry or otel so we can check the span instance
* to verify which one it is and remove this field entirely.
*
* @deprecated This field will be removed.
*/
public instrumenter: Instrumenter;

protected _traceId: string;
protected _spanId: string;
protected _parentSpanId?: string | undefined;
Expand Down Expand Up @@ -132,8 +119,6 @@ export class SentrySpan implements SpanInterface {
this.tags = spanContext.tags ? { ...spanContext.tags } : {};
// eslint-disable-next-line deprecation/deprecation
this.data = spanContext.data ? { ...spanContext.data } : {};
// eslint-disable-next-line deprecation/deprecation
this.instrumenter = spanContext.instrumenter || 'sentry';

this._attributes = {};
this.setAttributes({
Expand Down
1 change: 0 additions & 1 deletion packages/core/test/lib/serverruntimeclient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ function getDefaultClientOptions(options: Partial<ServerRuntimeClientOptions> =
integrations: [],
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => Promise.resolve({})),
stackParser: () => [],
instrumenter: 'sentry',
...options,
};
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
addTracingExtensions,
getActiveSpan,
getClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
spanToTraceHeader,
Expand Down Expand Up @@ -35,13 +34,11 @@ export function wrapAppGetInitialPropsWithSentry(origAppGetInitialProps: AppGetI
const { req, res } = context.ctx;

const errorWrappedAppGetInitialProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

// Generally we can assume that `req` and `res` are always defined on the server:
// https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
// This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher
// span with each other when there are no req or res objects, we simply do not trace them at all here.
if (req && res && options?.instrumenter === 'sentry') {
if (req && res) {
const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedAppGetInitialProps, req, res, {
dataFetcherRouteName: '/_app',
requestedRouteName: context.ctx.pathname,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { addTracingExtensions, getClient } from '@sentry/core';
import { addTracingExtensions } from '@sentry/core';
import type Document from 'next/document';

import { isBuild } from './utils/isBuild';
Expand Down Expand Up @@ -29,13 +29,11 @@ export function wrapDocumentGetInitialPropsWithSentry(
const { req, res } = context;

const errorWrappedGetInitialProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

// Generally we can assume that `req` and `res` are always defined on the server:
// https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
// This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher
// span with each other when there are no req or res objects, we simply do not trace them at all here.
if (req && res && options?.instrumenter === 'sentry') {
if (req && res) {
const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, {
dataFetcherRouteName: '/_document',
requestedRouteName: context.pathname,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
addTracingExtensions,
getActiveSpan,
getClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
spanToTraceHeader,
Expand Down Expand Up @@ -38,13 +37,11 @@ export function wrapErrorGetInitialPropsWithSentry(
const { req, res } = context;

const errorWrappedGetInitialProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

// Generally we can assume that `req` and `res` are always defined on the server:
// https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
// This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher
// span with each other when there are no req or res objects, we simply do not trace them at all here.
if (req && res && options?.instrumenter === 'sentry') {
if (req && res) {
const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, {
dataFetcherRouteName: '/_error',
requestedRouteName: context.pathname,
Expand Down
5 changes: 1 addition & 4 deletions packages/nextjs/src/common/wrapGetInitialPropsWithSentry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
addTracingExtensions,
getActiveSpan,
getClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
spanToTraceHeader,
Expand Down Expand Up @@ -34,13 +33,11 @@ export function wrapGetInitialPropsWithSentry(origGetInitialProps: GetInitialPro
const { req, res } = context;

const errorWrappedGetInitialProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

// Generally we can assume that `req` and `res` are always defined on the server:
// https://nextjs.org/docs/api-reference/data-fetching/get-initial-props#context-object
// This does not seem to be the case in dev mode. Because we have no clean way of associating the the data fetcher
// span with each other when there are no req or res objects, we simply do not trace them at all here.
if (req && res && options?.instrumenter === 'sentry') {
if (req && res) {
const tracedGetInitialProps = withTracedServerSideDataFetcher(errorWrappedGetInitialProps, req, res, {
dataFetcherRouteName: context.pathname,
requestedRouteName: context.pathname,
Expand Down
47 changes: 20 additions & 27 deletions packages/nextjs/src/common/wrapGetServerSidePropsWithSentry.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {
addTracingExtensions,
getActiveSpan,
getClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
spanToTraceHeader,
Expand Down Expand Up @@ -35,34 +34,28 @@ export function wrapGetServerSidePropsWithSentry(
const { req, res } = context;

const errorWrappedGetServerSideProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

if (options?.instrumenter === 'sentry') {
const tracedGetServerSideProps = withTracedServerSideDataFetcher(errorWrappedGetServerSideProps, req, res, {
dataFetcherRouteName: parameterizedRoute,
requestedRouteName: parameterizedRoute,
dataFetchingMethodName: 'getServerSideProps',
});

const serverSideProps = await (tracedGetServerSideProps.apply(thisArg, args) as ReturnType<
typeof tracedGetServerSideProps
>);

if (serverSideProps && 'props' in serverSideProps) {
const activeSpan = getActiveSpan();
const requestTransaction = getSpanFromRequest(req) ?? (activeSpan ? getRootSpan(activeSpan) : undefined);
if (requestTransaction) {
serverSideProps.props._sentryTraceData = spanToTraceHeader(requestTransaction);

const dynamicSamplingContext = getDynamicSamplingContextFromSpan(requestTransaction);
serverSideProps.props._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
}
const tracedGetServerSideProps = withTracedServerSideDataFetcher(errorWrappedGetServerSideProps, req, res, {
dataFetcherRouteName: parameterizedRoute,
requestedRouteName: parameterizedRoute,
dataFetchingMethodName: 'getServerSideProps',
});

const serverSideProps = await (tracedGetServerSideProps.apply(thisArg, args) as ReturnType<
typeof tracedGetServerSideProps
>);

if (serverSideProps && 'props' in serverSideProps) {
const activeSpan = getActiveSpan();
const requestTransaction = getSpanFromRequest(req) ?? (activeSpan ? getRootSpan(activeSpan) : undefined);
if (requestTransaction) {
serverSideProps.props._sentryTraceData = spanToTraceHeader(requestTransaction);

const dynamicSamplingContext = getDynamicSamplingContextFromSpan(requestTransaction);
serverSideProps.props._sentryBaggage = dynamicSamplingContextToSentryBaggageHeader(dynamicSamplingContext);
}

return serverSideProps;
} else {
return errorWrappedGetServerSideProps.apply(thisArg, args);
}

return serverSideProps;
},
});
}
14 changes: 5 additions & 9 deletions packages/nextjs/src/common/wrapGetStaticPropsWithSentry.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { addTracingExtensions, getClient } from '@sentry/core';
import { addTracingExtensions } from '@sentry/core';
import type { GetStaticProps } from 'next';

import { isBuild } from './utils/isBuild';
Expand Down Expand Up @@ -26,14 +26,10 @@ export function wrapGetStaticPropsWithSentry(
addTracingExtensions();

const errorWrappedGetStaticProps = withErrorInstrumentation(wrappingTarget);
const options = getClient()?.getOptions();

if (options?.instrumenter === 'sentry') {
return callDataFetcherTraced(errorWrappedGetStaticProps, args, {
parameterizedRoute,
dataFetchingMethodName: 'getStaticProps',
});
}
return callDataFetcherTraced(errorWrappedGetStaticProps, args, {
parameterizedRoute,
dataFetchingMethodName: 'getStaticProps',
});

return errorWrappedGetStaticProps.apply(thisArg, args);
},
Expand Down
2 changes: 1 addition & 1 deletion packages/nextjs/test/config/wrappers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe('data-fetching function wrappers should create spans', () => {
jest.spyOn(SentryCore, 'hasTracingEnabled').mockReturnValue(true);
jest.spyOn(SentryCore, 'getClient').mockImplementation(() => {
return {
getOptions: () => ({ instrumenter: 'sentry' }),
getOptions: () => ({}),
getDsn: () => {},
} as Client;
});
Expand Down
1 change: 0 additions & 1 deletion packages/node-experimental/src/sdk/init.ts
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ function getClientOptions(options: NodeOptions): NodeClientOptions {
...baseOptions,
...options,
...overwriteOptions,
instrumenter: 'otel',
stackParser: stackParserFromStackParserOptions(options.stackParser || defaultStackParser),
integrations: getIntegrationsToSetup({
defaultIntegrations: options.defaultIntegrations,
Expand Down
7 changes: 1 addition & 6 deletions packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,7 @@ export function tracingHandler(): (
): void {
const options = getClient()?.getOptions();

if (
!options ||
options.instrumenter !== 'sentry' ||
req.method?.toUpperCase() === 'OPTIONS' ||
req.method?.toUpperCase() === 'HEAD'
) {
if (req.method?.toUpperCase() === 'OPTIONS' || req.method?.toUpperCase() === 'HEAD') {
return next();
}

Expand Down
6 changes: 0 additions & 6 deletions packages/node/src/integrations/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,6 @@ export class Http implements Integration {
return;
}

// Do not auto-instrument for other instrumenter
if (clientOptions && clientOptions.instrumenter !== 'sentry') {
DEBUG_BUILD && logger.log('HTTP Integration is skipped because of instrumenter configuration.');
return;
}

const shouldCreateSpanForRequest = _getShouldCreateSpanForRequest(shouldCreateSpans, this._tracing, clientOptions);

// eslint-disable-next-line deprecation/deprecation
Expand Down
4 changes: 0 additions & 4 deletions packages/node/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,6 @@ export function init(options: NodeOptions = {}): void {
options.autoSessionTracking = true;
}

if (options.instrumenter === undefined) {
options.instrumenter = 'sentry';
}

// TODO(v7): Refactor this to reduce the logic above
const clientOptions: NodeClientOptions = {
...options,
Expand Down
1 change: 0 additions & 1 deletion packages/node/test/helper/node-client-options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export function getDefaultNodeClientOptions(options: Partial<NodeClientOptions>
integrations: [],
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})),
stackParser: () => [],
instrumenter: 'sentry',
...options,
};
}
19 changes: 0 additions & 19 deletions packages/node/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import {
import { setNodeAsyncContextStrategy } from '../src/async';
import { ContextLines } from '../src/integrations';
import { defaultStackParser, getDefaultIntegrations } from '../src/sdk';
import type { NodeClientOptions } from '../src/types';
import { getDefaultNodeClientOptions } from './helper/node-client-options';

jest.mock('@sentry/core', () => {
Expand Down Expand Up @@ -487,24 +486,6 @@ describe('SentryNode initialization', () => {
});
});

describe('instrumenter', () => {
it('defaults to sentry instrumenter', () => {
init({ dsn });

const instrumenter = (getClient()?.getOptions() as NodeClientOptions).instrumenter;

expect(instrumenter).toEqual('sentry');
});

it('allows to set instrumenter', () => {
init({ dsn, instrumenter: 'otel' });

const instrumenter = (getClient()?.getOptions() as NodeClientOptions).instrumenter;

expect(instrumenter).toEqual('otel');
});
});

describe('propagation context', () => {
beforeEach(() => {
process.env.SENTRY_TRACE = '12312012123120121231201212312012-1121201211212012-0';
Expand Down
29 changes: 1 addition & 28 deletions packages/node/test/integrations/http.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Transaction } from '@sentry/core';
import { getCurrentScope, setUser, spanToJSON, startInactiveSpan } from '@sentry/core';
import { addTracingExtensions } from '@sentry/core';
import type { TransactionContext } from '@sentry/types';
import { TRACEPARENT_REGEXP, logger } from '@sentry/utils';
import { TRACEPARENT_REGEXP } from '@sentry/utils';
import * as nock from 'nock';
import { HttpsProxyAgent } from '../../src/proxy';

Expand Down Expand Up @@ -259,33 +259,6 @@ describe('tracing', () => {
expect(baggage).not.toBeDefined();
});

it("doesn't attach when using otel instrumenter", () => {
const loggerLogSpy = jest.spyOn(logger, 'log');

const options = getDefaultNodeClientOptions({
dsn: 'https://[email protected]/12312012',
tracesSampleRate: 1.0,
// eslint-disable-next-line deprecation/deprecation
integrations: [new HttpIntegration({ tracing: true })],
release: '1.0.0',
environment: 'production',
instrumenter: 'otel',
});
const client = new NodeClient(options);
setCurrentClient(client);
// eslint-disable-next-line deprecation/deprecation
const hub = getCurrentHub();

// eslint-disable-next-line deprecation/deprecation
const integration = new HttpIntegration();
integration.setupOnce(
() => {},
() => hub as Hub,
);

expect(loggerLogSpy).toBeCalledWith('HTTP Integration is skipped because of instrumenter configuration.');
});

it('omits query and fragment from description and adds to span data instead', () => {
nock('http://dogs.are.great').get('/spaniel?tail=wag&cute=true#learn-more').reply(200);

Expand Down
Loading
Loading