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 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 .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 @@ -15,7 +15,7 @@ test('Should send a transaction with a fetch span', async ({ page }) => {
expect(transactionEvent.spans).toContainEqual(
expect.objectContaining({
data: expect.objectContaining({
'http.method': 'GET',
'http.request.method': 'GET',
'sentry.op': 'http.client',
'sentry.origin': 'auto.http.otel.node_fetch',
}),
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 @@ -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
3 changes: 2 additions & 1 deletion packages/bun/src/integrations/bunserver.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
captureException,
Expand Down Expand Up @@ -65,7 +66,7 @@ function instrumentBunServeOptions(serveOptions: Parameters<typeof Bun.serve>[0]
const parsedUrl = parseUrl(request.url);
const attributes: SpanAttributes = {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.bun.serve',
'http.request.method': request.method || 'GET',
[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: request.method || 'GET',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
};
if (parsedUrl.search) {
Expand Down
6 changes: 4 additions & 2 deletions packages/cloudflare/src/request.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import type { ExecutionContext, IncomingRequestCfProperties } from '@cloudflare/workers-types';

import {
SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
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.40.0",
"@opentelemetry/instrumentation-pg": "0.44.0",
"@opentelemetry/instrumentation-redis-4": "0.42.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
141 changes: 25 additions & 116 deletions packages/node/src/integrations/node-fetch.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,9 @@
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 {
addOpenTelemetryInstrumentation,
generateSpanContextForPropagationContext,
getPropagationContextFromSpan,
} from '@sentry/opentelemetry';
import type { UndiciRequest, UndiciResponse } from '@opentelemetry/instrumentation-undici';
import { UndiciInstrumentation } from '@opentelemetry/instrumentation-undici';
import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, addBreadcrumb, defineIntegration } from '@sentry/core';
import { addOpenTelemetryInstrumentation } 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,106 +23,38 @@ 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) {
return true;
}

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

// We generate a virtual span context from the active one,
// Where we attach the URL to the trace state, so the propagator can pick it up
const activeSpan = trace.getSpan(ctx);
const propagationContext = activeSpan
? getPropagationContextFromSpan(activeSpan)
: getCurrentScope().getPropagationContext();

const spanContext = generateSpanContextForPropagationContext(propagationContext);
// We know that in practice we'll _always_ haven a traceState here
spanContext.traceState = spanContext.traceState?.set('sentry.url', url);
const ctxWithUrlTraceState = trace.setSpanContext(ctx, spanContext);

propagation.inject(ctxWithUrlTraceState, addedHeaders);

const requestHeaders = request.headers;
if (Array.isArray(requestHeaders)) {
Object.entries(addedHeaders).forEach(headers => requestHeaders.push(...headers));
} else {
request.headers += Object.entries(addedHeaders)
.map(([k, v]) => `${k}: ${v}\r\n`)
.join('');
}

// Prevent starting a span for this request
return true;
}

return false;
return !!shouldIgnore;
},
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 +74,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
6 changes: 4 additions & 2 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 @@ -54,7 +56,7 @@ export class SentrySampler implements Sampler {
// 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
14 changes: 8 additions & 6 deletions packages/opentelemetry/src/utils/parseSpanDescription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,12 @@ 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_HTTP_REQUEST_METHOD,
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 @@ -45,10 +50,7 @@ export function inferSpanData(name: string, attributes: SpanAttributes, kind: Sp
}

// if http.method exists, this is an http request span
//
// TODO: Referencing `http.request.method` is a temporary workaround until the semantic
// conventions export an attribute key for it.
const httpMethod = attributes['http.request.method'] || attributes[SEMATTRS_HTTP_METHOD];
const httpMethod = attributes[SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD] || attributes[SEMATTRS_HTTP_METHOD];
if (httpMethod) {
return descriptionForHttpMethod({ attributes, name, kind }, httpMethod);
}
Expand Down Expand Up @@ -213,7 +215,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