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): Ignore outgoing Sentry HTTP requests from otel integration #6116

Merged
merged 1 commit into from Nov 2, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/opentelemetry-node/package.json
Expand Up @@ -33,7 +33,8 @@
"@opentelemetry/core": "^1.7.0",
"@opentelemetry/sdk-trace-base": "^1.7.0",
"@opentelemetry/sdk-trace-node": "^1.7.0",
"@opentelemetry/semantic-conventions": "^1.7.0"
"@opentelemetry/semantic-conventions": "^1.7.0",
"@sentry/node": "7.17.3"
},
"scripts": {
"build": "run-p build:rollup build:types",
Expand Down
14 changes: 11 additions & 3 deletions packages/opentelemetry-node/src/spanprocessor.ts
Expand Up @@ -5,6 +5,7 @@ import { Transaction } from '@sentry/tracing';
import { Span as SentrySpan, TransactionContext } from '@sentry/types';
import { logger } from '@sentry/utils';

import { isSentryRequestSpan } from './utils/is-sentry-request';
import { mapOtelStatus } from './utils/map-otel-status';
import { parseSpanDescription } from './utils/parse-otel-span-description';

Expand All @@ -31,9 +32,6 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
return;
}

// TODO: handle sentry requests
// if isSentryRequest(otelSpan) return;

const otelSpanId = otelSpan.spanContext().spanId;
const otelParentSpanId = otelSpan.parentSpanId;

Expand Down Expand Up @@ -77,6 +75,16 @@ export class SentrySpanProcessor implements OtelSpanProcessor {
return;
}

// Auto-instrumentation often captures outgoing HTTP requests
// This means that Sentry HTTP requests created by this integration can, in turn, be captured by OTEL auto instrumentation,
// leading to an infinite loop.
// In this case, we do not want to finish the span, in order to avoid sending it to Sentry
if (isSentryRequestSpan(otelSpan)) {
// Make sure to remove any references, so this can be GCed
this._map.delete(otelSpanId);
return;
}

if (sentrySpan instanceof Transaction) {
updateTransactionWithOtelData(sentrySpan, otelSpan);
finishTransactionWithContextFromOtelData(sentrySpan, otelSpan);
Expand Down
29 changes: 29 additions & 0 deletions packages/opentelemetry-node/src/utils/is-sentry-request.ts
@@ -0,0 +1,29 @@
import { Span as OtelSpan } from '@opentelemetry/sdk-trace-base';
import { SemanticAttributes } from '@opentelemetry/semantic-conventions';
import { getCurrentHub } from '@sentry/core';

/**
*
* @param otelSpan Checks wheter a given OTEL Span is an http request to sentry.
* @returns boolean
*/
export function isSentryRequestSpan(otelSpan: OtelSpan): boolean {
const { attributes } = otelSpan;

const httpUrl = attributes[SemanticAttributes.HTTP_URL];

if (!httpUrl) {
return false;
}

return isSentryRequestUrl(httpUrl.toString());
}

/**
* Checks whether given url points to Sentry server
* @param url url to verify
*/
function isSentryRequestUrl(url: string): boolean {
const dsn = getCurrentHub().getClient()?.getDsn();
return dsn ? url.includes(dsn.host) : false;
}
114 changes: 112 additions & 2 deletions packages/opentelemetry-node/test/spanprocessor.test.ts
Expand Up @@ -4,12 +4,16 @@ import { Resource } from '@opentelemetry/resources';
import { Span as OtelSpan } from '@opentelemetry/sdk-trace-base';
import { NodeTracerProvider } from '@opentelemetry/sdk-trace-node';
import { SemanticAttributes, SemanticResourceAttributes } from '@opentelemetry/semantic-conventions';
import { Hub, makeMain } from '@sentry/core';
import { createTransport, Hub, makeMain } from '@sentry/core';
import { NodeClient } from '@sentry/node';
import { addExtensionMethods, Span as SentrySpan, SpanStatusType, Transaction } from '@sentry/tracing';
import { Contexts, Scope } from '@sentry/types';
import { resolvedSyncPromise } from '@sentry/utils';

import { SentrySpanProcessor } from '../src/spanprocessor';

const SENTRY_DSN = 'https://0@0.ingest.sentry.io/0';

// Integration Test of SentrySpanProcessor

beforeAll(() => {
Expand All @@ -22,7 +26,13 @@ describe('SentrySpanProcessor', () => {
let spanProcessor: SentrySpanProcessor;

beforeEach(() => {
hub = new Hub();
const client = new NodeClient({
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind either way - using a mock client was faster for me, but I'm fine with creating a NodeClient here.

dsn: SENTRY_DSN,
integrations: [],
transport: () => createTransport({ recordDroppedEvent: () => undefined }, _ => resolvedSyncPromise({})),
stackParser: () => [],
});
hub = new Hub(client);
makeMain(hub);

spanProcessor = new SentrySpanProcessor();
Expand Down Expand Up @@ -561,6 +571,106 @@ describe('SentrySpanProcessor', () => {
});
});
});

describe('skip sentry requests', () => {
it('does not finish transaction for Sentry request', async () => {
const otelSpan = provider.getTracer('default').startSpan('POST to sentry', {
attributes: {
[SemanticAttributes.HTTP_METHOD]: 'POST',
[SemanticAttributes.HTTP_URL]: `${SENTRY_DSN}/sub/route`,
},
}) as OtelSpan;

const sentrySpanTransaction = getSpanForOtelSpan(otelSpan) as Transaction | undefined;
expect(sentrySpanTransaction).toBeDefined();

otelSpan.end();

expect(sentrySpanTransaction?.endTimestamp).toBeUndefined();

// Ensure it is still removed from map!
expect(getSpanForOtelSpan(otelSpan)).toBeUndefined();
});

it('finishes transaction for non-Sentry request', async () => {
const otelSpan = provider.getTracer('default').startSpan('POST to sentry', {
attributes: {
[SemanticAttributes.HTTP_METHOD]: 'POST',
[SemanticAttributes.HTTP_URL]: 'https://other.sentry.io/sub/route',
},
}) as OtelSpan;

const sentrySpanTransaction = getSpanForOtelSpan(otelSpan) as Transaction | undefined;
expect(sentrySpanTransaction).toBeDefined();

otelSpan.end();

expect(sentrySpanTransaction?.endTimestamp).toBeDefined();
});

it('does not finish spans for Sentry request', async () => {
const tracer = provider.getTracer('default');

tracer.startActiveSpan('GET /users', () => {
tracer.startActiveSpan(
'SELECT * FROM users;',
{
attributes: {
[SemanticAttributes.HTTP_METHOD]: 'POST',
[SemanticAttributes.HTTP_URL]: `${SENTRY_DSN}/sub/route`,
},
},
child => {
const childOtelSpan = child as OtelSpan;

const sentrySpan = getSpanForOtelSpan(childOtelSpan);
expect(sentrySpan).toBeDefined();

childOtelSpan.end();

expect(sentrySpan?.endTimestamp).toBeUndefined();

// Ensure it is still removed from map!
expect(getSpanForOtelSpan(childOtelSpan)).toBeUndefined();
},
);
});
});

it('handles child spans of Sentry requests normally', async () => {
const tracer = provider.getTracer('default');

tracer.startActiveSpan('GET /users', () => {
tracer.startActiveSpan(
'SELECT * FROM users;',
{
attributes: {
[SemanticAttributes.HTTP_METHOD]: 'POST',
[SemanticAttributes.HTTP_URL]: `${SENTRY_DSN}/sub/route`,
},
},
child => {
const childOtelSpan = child as OtelSpan;

const grandchildSpan = tracer.startSpan('child 1');

const sentrySpan = getSpanForOtelSpan(childOtelSpan);
expect(sentrySpan).toBeDefined();

const sentryGrandchildSpan = getSpanForOtelSpan(grandchildSpan);
expect(sentryGrandchildSpan).toBeDefined();

grandchildSpan.end();

childOtelSpan.end();

expect(sentryGrandchildSpan?.endTimestamp).toBeDefined();
expect(sentrySpan?.endTimestamp).toBeUndefined();
},
);
});
});
});
});

// OTEL expects a custom date format
Expand Down