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(node): Use @opentelemetry/instrumentation-undici for fetch tracing #13485

Merged
merged 21 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from 12 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 .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ updates:
- dependency-name: "@sentry/esbuild-plugin"
- dependency-name: "@opentelemetry/*"
- dependency-name: "@prisma/instrumentation"
- dependency-name: "opentelemetry-instrumentation-fetch-node"
versioning-strategy: increase
commit-message:
prefix: feat
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils';
import { createRunner } from '../../../../utils/runner';
import { createTestServer } from '../../../../utils/server';

conditionalTest({ min: 18 })('outgoing fetch', () => {
conditionalTest({ min: 16 })('outgoing fetch', () => {
test('outgoing fetch requests are correctly instrumented with tracing disabled', done => {
expect.assertions(11);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ Sentry.init({
});

async function run(): Promise<void> {
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
await new Promise(resolve => setTimeout(resolve, 100));
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils';
import { createRunner } from '../../../../utils/runner';
import { createTestServer } from '../../../../utils/server';

conditionalTest({ min: 18 })('outgoing fetch', () => {
conditionalTest({ min: 16 })('outgoing fetch', () => {
test('outgoing sampled fetch requests without active span are correctly instrumented', done => {
expect.assertions(11);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ Sentry.init({
async function run(): Promise<void> {
// Wrap in span that is not sampled
await Sentry.startSpan({ name: 'outer' }, async () => {
// Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
await new Promise(resolve => setTimeout(resolve, 100));
await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { conditionalTest } from '../../../../utils';
import { createRunner } from '../../../../utils/runner';
import { createTestServer } from '../../../../utils/server';

conditionalTest({ min: 18 })('outgoing fetch', () => {
conditionalTest({ min: 16 })('outgoing fetch', () => {
test('outgoing fetch requests are correctly instrumented when not sampled', done => {
expect.assertions(11);

Expand Down
6 changes: 4 additions & 2 deletions packages/cloudflare/src/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
SEMANTIC_ATTRIBUTE_URL_FULL,
captureException,
continueTrace,
flush,
Expand Down Expand Up @@ -45,8 +47,8 @@ export function wrapRequestHandler(
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.cloudflare',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'http.server',
['http.request.method']: request.method,
['url.full']: request.url,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I replaced these in a couple of places since I just added temporary constants for them

[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method,
[SEMANTIC_ATTRIBUTE_URL_FULL]: request.url,
};

const contentLength = request.headers.get('content-length');
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/semanticAttributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit';
export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key';

export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size';

/** TODO: Remove these once we update to latest semantic conventions */
export const SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD = 'http.request.method';
export const SEMANTIC_ATTRIBUTE_URL_FULL = 'url.full';
4 changes: 1 addition & 3 deletions packages/node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
"@opentelemetry/instrumentation-nestjs-core": "0.39.0",
"@opentelemetry/instrumentation-pg": "0.43.0",
"@opentelemetry/instrumentation-redis-4": "0.41.0",
"@opentelemetry/instrumentation-undici": "0.5.0",
"@opentelemetry/resources": "^1.25.1",
"@opentelemetry/sdk-trace-base": "^1.25.1",
"@opentelemetry/semantic-conventions": "^1.25.1",
Expand All @@ -99,9 +100,6 @@
"devDependencies": {
"@types/node": "^14.18.0"
},
"optionalDependencies": {
"opentelemetry-instrumentation-fetch-node": "1.2.3"
},
"scripts": {
"build": "run-p build:transpile build:types",
"build:dev": "yarn build",
Expand Down
102 changes: 30 additions & 72 deletions packages/node/src/integrations/node-fetch.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,21 @@
import type { Span } from '@opentelemetry/api';
import { trace } from '@opentelemetry/api';
import { context, propagation } from '@opentelemetry/api';
import { addBreadcrumb, defineIntegration, getCurrentScope, hasTracingEnabled } from '@sentry/core';
import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici';
import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici';
import {
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
addBreadcrumb,
defineIntegration,
getCurrentScope,
hasTracingEnabled,
} from '@sentry/core';
import {
addOpenTelemetryInstrumentation,
generateSpanContextForPropagationContext,
getPropagationContextFromSpan,
} from '@sentry/opentelemetry';
import type { IntegrationFn, SanitizedRequestData } from '@sentry/types';
import { getSanitizedUrlString, logger, parseUrl } from '@sentry/utils';
import { DEBUG_BUILD } from '../debug-build';
import { NODE_MAJOR } from '../nodeVersion';

import type { FetchInstrumentation } from 'opentelemetry-instrumentation-fetch-node';

import { addOriginToSpan } from '../utils/addOriginToSpan';

interface FetchRequest {
method: string;
origin: string;
path: string;
headers: string | string[];
}

interface FetchResponse {
headers: Buffer[];
statusCode: number;
}
import { getSanitizedUrlString, parseUrl } from '@sentry/utils';

interface NodeFetchOptions {
/**
Expand All @@ -46,32 +35,13 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => {
const _breadcrumbs = typeof options.breadcrumbs === 'undefined' ? true : options.breadcrumbs;
const _ignoreOutgoingRequests = options.ignoreOutgoingRequests;

async function getInstrumentation(): Promise<FetchInstrumentation | void> {
// Only add NodeFetch if Node >= 18, as previous versions do not support it
if (NODE_MAJOR < 18) {
DEBUG_BUILD && logger.log('NodeFetch is not supported on Node < 18, skipping instrumentation...');
return;
timfish marked this conversation as resolved.
Show resolved Hide resolved
}

try {
const pkg = await import('opentelemetry-instrumentation-fetch-node');
const { FetchInstrumentation } = pkg;

class SentryNodeFetchInstrumentation extends FetchInstrumentation {
// We extend this method so we have access to request _and_ response for the breadcrumb
public onHeaders({ request, response }: { request: FetchRequest; response: FetchResponse }): void {
if (_breadcrumbs) {
_addRequestBreadcrumb(request, response);
}

return super.onHeaders({ request, response });
}
}

return new SentryNodeFetchInstrumentation({
ignoreRequestHook: (request: FetchRequest) => {
return {
name: 'NodeFetch',
setupOnce() {
const instrumentation = new UndiciInstrumentation({
requireParentforSpans: false,
ignoreRequestHook: request => {
Copy link
Member

Choose a reason for hiding this comment

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

is this a typo in our config or upstream, seems like it should be requireParentForSpans 😅

Copy link
Collaborator Author

@timfish timfish Sep 5, 2024

Choose a reason for hiding this comment

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

const url = getAbsoluteUrl(request.origin, request.path);
const tracingDisabled = !hasTracingEnabled();
const shouldIgnore = _ignoreOutgoingRequests && url && _ignoreOutgoingRequests(url);

if (shouldIgnore) {
Expand All @@ -80,7 +50,7 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => {

// If tracing is disabled, we still want to propagate traces
// So we do that manually here, matching what the instrumentation does otherwise
if (tracingDisabled) {
if (!hasTracingEnabled()) {
const ctx = context.active();
timfish marked this conversation as resolved.
Show resolved Hide resolved
const addedHeaders: Record<string, string> = {};

Expand Down Expand Up @@ -113,39 +83,27 @@ const _nativeNodeFetchIntegration = ((options: NodeFetchOptions = {}) => {

return false;
},
onRequest: ({ span }: { span: Span }) => {
_updateSpan(span);
startSpanHook: () => {
return {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.otel.node_fetch',
};
},
responseHook: (_, { request, response }) => {
if (_breadcrumbs) {
addRequestBreadcrumb(request, response);
}
},
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} as any);
} catch (error) {
// Could not load instrumentation
DEBUG_BUILD && logger.log('Error while loading NodeFetch instrumentation: \n', error);
}
}

return {
name: 'NodeFetch',
setupOnce() {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
getInstrumentation().then(instrumentation => {
if (instrumentation) {
addOpenTelemetryInstrumentation(instrumentation);
}
});

addOpenTelemetryInstrumentation(instrumentation);
},
};
}) satisfies IntegrationFn;

export const nativeNodeFetchIntegration = defineIntegration(_nativeNodeFetchIntegration);

/** Update the span with data we need. */
function _updateSpan(span: Span): void {
addOriginToSpan(span, 'auto.http.otel.node_fetch');
}

/** Add a breadcrumb for outgoing requests. */
function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse): void {
function addRequestBreadcrumb(request: UndiciRequest, response: UndiciResponse): void {
const data = getBreadcrumbData(request);

addBreadcrumb(
Expand All @@ -165,7 +123,7 @@ function _addRequestBreadcrumb(request: FetchRequest, response: FetchResponse):
);
}

function getBreadcrumbData(request: FetchRequest): Partial<SanitizedRequestData> {
function getBreadcrumbData(request: UndiciRequest): Partial<SanitizedRequestData> {
try {
const url = new URL(request.path, request.origin);
const parsedUrl = parseUrl(url.toString());
Expand Down
4 changes: 3 additions & 1 deletion packages/opentelemetry/src/propagator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { propagation, trace } from '@opentelemetry/api';
import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions';
import type { continueTrace } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_URL_FULL } from '@sentry/core';
import { hasTracingEnabled } from '@sentry/core';
import { getRootSpan } from '@sentry/core';
import { spanToJSON } from '@sentry/core';
Expand Down Expand Up @@ -292,7 +293,8 @@ function getExistingBaggage(carrier: unknown): string | undefined {
* 2. Else, if the active span has no URL attribute (e.g. it is unsampled), we check a special trace state (which we set in our sampler).
*/
function getCurrentURL(span: Span): string | undefined {
const urlAttribute = spanToJSON(span).data?.[SEMATTRS_HTTP_URL];
const spanData = spanToJSON(span).data;
const urlAttribute = spanData?.[SEMATTRS_HTTP_URL] || spanData?.[SEMANTIC_ATTRIBUTE_URL_FULL];
if (urlAttribute) {
return urlAttribute;
}
Expand Down
8 changes: 5 additions & 3 deletions packages/opentelemetry/src/sampler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import { TraceState } from '@opentelemetry/core';
import type { Sampler, SamplingResult } from '@opentelemetry/sdk-trace-base';
import { SamplingDecision } from '@opentelemetry/sdk-trace-base';
import {
SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
SEMANTIC_ATTRIBUTE_URL_FULL,
hasTracingEnabled,
sampleSpan,
} from '@sentry/core';
Expand Down Expand Up @@ -50,11 +52,11 @@ export class SentrySampler implements Sampler {
return wrapSamplingDecision({ decision: undefined, context, spanAttributes });
}

// If we have a http.client span that has no local parent, we never want to sample it
// If we have a http.client or undici span that has no local parent, we never want to sample it
timfish marked this conversation as resolved.
Show resolved Hide resolved
// but we want to leave downstream sampling decisions up to the server
if (
spanKind === SpanKind.CLIENT &&
spanAttributes[SEMATTRS_HTTP_METHOD] &&
(spanAttributes[SEMATTRS_HTTP_METHOD] || spanAttributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]) &&
(!parentSpan || parentContext?.isRemote)
) {
return wrapSamplingDecision({ decision: undefined, context, spanAttributes });
Expand Down Expand Up @@ -196,7 +198,7 @@ function getBaseTraceState(context: Context, spanAttributes: SpanAttributes): Tr
let traceState = parentContext?.traceState || new TraceState();

// We always keep the URL on the trace state, so we can access it in the propagator
const url = spanAttributes[SEMATTRS_HTTP_URL];
const url = spanAttributes[SEMATTRS_HTTP_URL] || spanAttributes[SEMANTIC_ATTRIBUTE_URL_FULL];
if (url && typeof url === 'string') {
traceState = traceState.set(SENTRY_TRACE_STATE_URL, url);
}
Expand Down
4 changes: 2 additions & 2 deletions packages/opentelemetry/src/utils/isSentryRequest.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions';
import { getClient, isSentryRequestUrl } from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_URL_FULL, getClient, isSentryRequestUrl } from '@sentry/core';

import type { AbstractSpan } from '../types';
import { spanHasAttributes } from './spanTypes';
Expand All @@ -16,7 +16,7 @@ export function isSentryRequestSpan(span: AbstractSpan): boolean {

const { attributes } = span;

const httpUrl = attributes[SEMATTRS_HTTP_URL];
const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL];

if (!httpUrl) {
return false;
Expand Down
8 changes: 6 additions & 2 deletions packages/opentelemetry/src/utils/parseSpanDescription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,11 @@ import {
import type { SpanAttributes, TransactionSource } from '@sentry/types';
import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';

import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core';
import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_URL_FULL,
} from '@sentry/core';
import { SEMANTIC_ATTRIBUTE_SENTRY_GRAPHQL_OPERATION } from '../semanticAttributes';
import type { AbstractSpan } from '../types';
import { getSpanKind } from './getSpanKind';
Expand Down Expand Up @@ -213,7 +217,7 @@ export function getSanitizedUrl(
// This is the relative path of the URL, e.g. /sub
const httpTarget = attributes[SEMATTRS_HTTP_TARGET];
// This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar
const httpUrl = attributes[SEMATTRS_HTTP_URL];
const httpUrl = attributes[SEMATTRS_HTTP_URL] || attributes[SEMANTIC_ATTRIBUTE_URL_FULL];
// This is the normalized route name - may not always be available!
const httpRoute = attributes[SEMATTRS_HTTP_ROUTE];

Expand Down
Loading
Loading