From 7d92ca6cf74f05a60b9de0af12810816c1a10abd Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 4 Sep 2023 13:01:08 +0200 Subject: [PATCH] fix(node-experimental): Ensure http breadcrumbs are correctly added Due to the way OTEL handles context (=scope/hub), I noticed that breadcrumbs added in the http integration are not correctly captured. The reason is that OTEL forks the hub for the http request context, and runs the hooks inside of this forked context. In order to deal with this, we have to actually attach the breadcrumb to the _parent_ Hub in this specific case. --- .../src/integrations/http.ts | 104 ++++++++++-------- .../src/sdk/otelContextManager.ts | 8 +- .../src/utils/getParentHub.ts | 21 ++++ .../src/utils/getRequestSpanData.ts | 7 +- 4 files changed, 85 insertions(+), 55 deletions(-) create mode 100644 packages/node-experimental/src/utils/getParentHub.ts diff --git a/packages/node-experimental/src/integrations/http.ts b/packages/node-experimental/src/integrations/http.ts index 5f45334da290..3fea68fbf8ec 100644 --- a/packages/node-experimental/src/integrations/http.ts +++ b/packages/node-experimental/src/integrations/http.ts @@ -11,6 +11,7 @@ import type { EventProcessor, Hub, Integration } from '@sentry/types'; import type { ClientRequest, IncomingMessage, ServerResponse } from 'http'; import type { NodeExperimentalClient } from '../sdk/client'; +import { getParentHub } from '../utils/getParentHub'; import { getRequestSpanData } from '../utils/getRequestSpanData'; interface TracingOptions { @@ -95,8 +96,11 @@ export class Http implements Integration { new HttpInstrumentation({ requireParentforOutgoingSpans: true, requireParentforIncomingSpans: false, - applyCustomAttributesOnSpan: (span, req, res) => { - this._onSpan(span as unknown as OtelSpan, req, res); + requestHook: (span, req) => { + this._updateSentrySpan(span as unknown as OtelSpan, req); + }, + responseHook: (span, res) => { + this._addRequestBreadcrumb(span as unknown as OtelSpan, res); }, }), ], @@ -136,64 +140,68 @@ export class Http implements Integration { return; }; - /** Handle an emitted span from the HTTP instrumentation. */ - private _onSpan( - span: OtelSpan, - request: ClientRequest | IncomingMessage, - response: IncomingMessage | ServerResponse, - ): void { - const data = getRequestSpanData(span, request, response); + /** Update the Sentry span data based on the OTEL span. */ + private _updateSentrySpan(span: OtelSpan, request: ClientRequest | IncomingMessage): void { + const data = getRequestSpanData(span); const { attributes } = span; const sentrySpan = _INTERNAL_getSentrySpan(span.spanContext().spanId); - if (sentrySpan) { - sentrySpan.origin = 'auto.http.otel.http'; + if (!sentrySpan) { + return; + } - const additionalData: Record = { - url: data.url, - }; + sentrySpan.origin = 'auto.http.otel.http'; - if (sentrySpan instanceof Transaction && span.kind === SpanKind.SERVER) { - sentrySpan.setMetadata({ request }); - } + const additionalData: Record = { + url: data.url, + }; - if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) { - const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string; - additionalData['http.response.status_code'] = statusCode; + if (sentrySpan instanceof Transaction && span.kind === SpanKind.SERVER) { + sentrySpan.setMetadata({ request }); + } - sentrySpan.setTag('http.status_code', statusCode); - } + if (attributes[SemanticAttributes.HTTP_STATUS_CODE]) { + const statusCode = attributes[SemanticAttributes.HTTP_STATUS_CODE] as string; + additionalData['http.response.status_code'] = statusCode; - if (data['http.query']) { - additionalData['http.query'] = data['http.query'].slice(1); - } - if (data['http.fragment']) { - additionalData['http.fragment'] = data['http.fragment'].slice(1); - } + sentrySpan.setTag('http.status_code', statusCode); + } - Object.keys(additionalData).forEach(prop => { - const value = additionalData[prop]; - sentrySpan.setData(prop, value); - }); + if (data['http.query']) { + additionalData['http.query'] = data['http.query'].slice(1); + } + if (data['http.fragment']) { + additionalData['http.fragment'] = data['http.fragment'].slice(1); } - if (this._breadcrumbs) { - getCurrentHub().addBreadcrumb( - { - category: 'http', - data: { - status_code: response.statusCode, - ...data, - }, - type: 'http', - }, - { - event: 'response', - request, - response, - }, - ); + Object.keys(additionalData).forEach(prop => { + const value = additionalData[prop]; + sentrySpan.setData(prop, value); + }); + } + + /** Add a breadcrumb for outgoing requests. */ + private _addRequestBreadcrumb(span: OtelSpan, response: IncomingMessage | ServerResponse): void { + if (!this._breadcrumbs || span.kind !== SpanKind.CLIENT) { + return; } + + const data = getRequestSpanData(span); + getParentHub().addBreadcrumb( + { + category: 'http', + data: { + status_code: response.statusCode, + ...data, + }, + type: 'http', + }, + { + event: 'response', + // request, ??? + response, + }, + ); } } diff --git a/packages/node-experimental/src/sdk/otelContextManager.ts b/packages/node-experimental/src/sdk/otelContextManager.ts index 9110b9e62328..eead211d83ca 100644 --- a/packages/node-experimental/src/sdk/otelContextManager.ts +++ b/packages/node-experimental/src/sdk/otelContextManager.ts @@ -5,6 +5,7 @@ import type { Carrier, Hub } from '@sentry/core'; import { ensureHubOnCarrier, getCurrentHub, getHubFromCarrier } from '@sentry/core'; export const OTEL_CONTEXT_HUB_KEY = api.createContextKey('sentry_hub'); +export const OTEL_CONTEXT_PARENT_KEY = api.createContextKey('sentry_parent_context'); function createNewHub(parent: Hub | undefined): Hub { const carrier: Carrier = {}; @@ -33,6 +34,11 @@ export class SentryContextManager extends AsyncLocalStorageContextManager { const existingHub = getCurrentHub(); const newHub = createNewHub(existingHub); - return super.with(context.setValue(OTEL_CONTEXT_HUB_KEY, newHub), fn, thisArg, ...args); + return super.with( + context.setValue(OTEL_CONTEXT_HUB_KEY, newHub).setValue(OTEL_CONTEXT_PARENT_KEY, context), + fn, + thisArg, + ...args, + ); } } diff --git a/packages/node-experimental/src/utils/getParentHub.ts b/packages/node-experimental/src/utils/getParentHub.ts new file mode 100644 index 000000000000..4579e4b8bb27 --- /dev/null +++ b/packages/node-experimental/src/utils/getParentHub.ts @@ -0,0 +1,21 @@ +import * as api from '@opentelemetry/api'; +import { getCurrentHub } from '@sentry/core'; +import type { Hub } from '@sentry/types'; + +import { OTEL_CONTEXT_HUB_KEY, OTEL_CONTEXT_PARENT_KEY } from '../sdk/otelContextManager'; + +/** + * Get the hub of the parent context. + * Can be useful if you e.g. need to add a breadcrumb from inside of an ongoing span. + * + * Without this, if you e.g. try to add a breadcrumb from an instrumentation's `onSpan` callback or similar, + * you'll always attach it to the current hub, which in that case will be hyper specific to the instrumentation. + */ +export function getParentHub(): Hub { + const ctx = api.context.active(); + const parentContext = ctx?.getValue(OTEL_CONTEXT_PARENT_KEY) as api.Context | undefined; + + const parentHub = parentContext && (parentContext.getValue(OTEL_CONTEXT_HUB_KEY) as Hub | undefined); + + return parentHub || getCurrentHub(); +} diff --git a/packages/node-experimental/src/utils/getRequestSpanData.ts b/packages/node-experimental/src/utils/getRequestSpanData.ts index 8d5d45f5f907..d238077bd9df 100644 --- a/packages/node-experimental/src/utils/getRequestSpanData.ts +++ b/packages/node-experimental/src/utils/getRequestSpanData.ts @@ -2,16 +2,11 @@ import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { SanitizedRequestData } from '@sentry/types'; import { getSanitizedUrlString, parseUrl } from '@sentry/utils'; -import type { ClientRequest, IncomingMessage, ServerResponse } from 'http'; /** * Get sanitizied request data from an OTEL span. */ -export function getRequestSpanData( - span: OtelSpan, - _request: ClientRequest | IncomingMessage, - _response: IncomingMessage | ServerResponse, -): SanitizedRequestData { +export function getRequestSpanData(span: OtelSpan): SanitizedRequestData { const data: SanitizedRequestData = { url: span.attributes[SemanticAttributes.HTTP_URL] as string, 'http.method': (span.attributes[SemanticAttributes.HTTP_METHOD] as string) || 'GET',