From ba92086432b9bd9e1a43932db0731a71e4aefcff Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Mar 2024 15:02:43 +0000 Subject: [PATCH 1/2] ref(node): Use new performance APIs in legacy `http` & `undici` --- packages/node/src/integrations/http.ts | 23 ++++++------ .../node/src/integrations/undici/index.ts | 35 ++++++++++--------- packages/node/test/integrations/http.test.ts | 25 +++++++------ 3 files changed, 42 insertions(+), 41 deletions(-) diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index 941682ee0290..a8ca8b832455 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -1,11 +1,11 @@ /* eslint-disable max-lines */ import type * as http from 'http'; import type * as https from 'https'; -import type { Hub, SentrySpan } from '@sentry/core'; +import type { Hub } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; import { defineIntegration, getIsolationScope, hasTracingEnabled } from '@sentry/core'; import { addBreadcrumb, - getActiveSpan, getClient, getCurrentHub, getCurrentScope, @@ -319,17 +319,18 @@ function _createWrappedRequestMethodFactory( const scope = getCurrentScope(); const isolationScope = getIsolationScope(); - const parentSpan = getActiveSpan() as SentrySpan; - const data = getRequestSpanData(requestUrl, requestOptions); + const attributes = getRequestSpanData(requestUrl, requestOptions); const requestSpan = shouldCreateSpan(rawRequestUrl) - ? // eslint-disable-next-line deprecation/deprecation - parentSpan?.startChild({ + ? startInactiveSpan({ + onlyIfParent: true, op: 'http.client', - origin: 'auto.http.node.http', - name: `${data['http.method']} ${data.url}`, - data, + name: `${attributes['http.method']} ${attributes.url}`, + attributes: { + ...attributes, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.node.http', + }, }) : undefined; @@ -365,7 +366,7 @@ function _createWrappedRequestMethodFactory( // eslint-disable-next-line @typescript-eslint/no-this-alias const req = this; if (breadcrumbsEnabled) { - addRequestBreadcrumb('response', data, req, res); + addRequestBreadcrumb('response', attributes, req, res); } if (requestSpan) { if (res.statusCode) { @@ -380,7 +381,7 @@ function _createWrappedRequestMethodFactory( const req = this; if (breadcrumbsEnabled) { - addRequestBreadcrumb('error', data, req); + addRequestBreadcrumb('error', attributes, req); } if (requestSpan) { setHttpStatus(requestSpan, 500); diff --git a/packages/node/src/integrations/undici/index.ts b/packages/node/src/integrations/undici/index.ts index 001f05814745..76d94947ee2d 100644 --- a/packages/node/src/integrations/undici/index.ts +++ b/packages/node/src/integrations/undici/index.ts @@ -1,9 +1,8 @@ -import type { SentrySpan } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; import { SPAN_STATUS_ERROR, addBreadcrumb, defineIntegration, - getActiveSpan, getClient, getCurrentScope, getDynamicSamplingContextFromClient, @@ -14,7 +13,14 @@ import { setHttpStatus, spanToTraceHeader, } from '@sentry/core'; -import type { EventProcessor, Integration, IntegrationFn, IntegrationFnResult, Span } from '@sentry/types'; +import type { + EventProcessor, + Integration, + IntegrationFn, + IntegrationFnResult, + Span, + SpanAttributes, +} from '@sentry/types'; import { LRUMap, dynamicSamplingContextToSentryBaggageHeader, @@ -185,9 +191,8 @@ export class Undici implements Integration { const clientOptions = client.getOptions(); const scope = getCurrentScope(); const isolationScope = getIsolationScope(); - const parentSpan = getActiveSpan() as SentrySpan; - const span = this._shouldCreateSpan(stringUrl) ? createRequestSpan(parentSpan, request, stringUrl) : undefined; + const span = this._shouldCreateSpan(stringUrl) ? createRequestSpan(request, stringUrl) : undefined; if (span) { request.__sentry_span__ = span; } @@ -326,28 +331,24 @@ function setHeadersOnRequest( } } -function createRequestSpan( - activeSpan: SentrySpan | undefined, - request: RequestWithSentry, - stringUrl: string, -): Span | undefined { +function createRequestSpan(request: RequestWithSentry, stringUrl: string): Span { const url = parseUrl(stringUrl); const method = request.method || 'GET'; - const data: Record = { + const attributes: SpanAttributes = { 'http.method': method, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.node.undici', }; if (url.search) { - data['http.query'] = url.search; + attributes['http.query'] = url.search; } if (url.hash) { - data['http.fragment'] = url.hash; + attributes['http.fragment'] = url.hash; } - // eslint-disable-next-line deprecation/deprecation - return activeSpan?.startChild({ + return startInactiveSpan({ + onlyIfParent: true, op: 'http.client', - origin: 'auto.http.node.undici', name: `${method} ${getSanitizedUrlString(url)}`, - data, + attributes, }); } diff --git a/packages/node/test/integrations/http.test.ts b/packages/node/test/integrations/http.test.ts index 62f7aaed1edc..dd5d3013af9c 100644 --- a/packages/node/test/integrations/http.test.ts +++ b/packages/node/test/integrations/http.test.ts @@ -1,6 +1,6 @@ import * as http from 'http'; import * as https from 'https'; -import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getSpanDescendants } from '@sentry/core'; +import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getSpanDescendants, startSpan } from '@sentry/core'; import type { Hub } from '@sentry/core'; import { getCurrentHub, getIsolationScope, setCurrentClient } from '@sentry/core'; import { Transaction } from '@sentry/core'; @@ -12,6 +12,7 @@ import * as nock from 'nock'; import { HttpsProxyAgent } from '../../src/proxy'; import type { Breadcrumb } from '../../src'; +import { _setSpanForScope } from '../../src/_setSpanForScope'; import { NodeClient } from '../../src/client'; import { Http as HttpIntegration, @@ -33,8 +34,7 @@ describe('tracing', () => { }); afterEach(() => { - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(undefined); + _setSpanForScope(getCurrentScope(), undefined); }); function createTransactionOnScope( @@ -55,9 +55,7 @@ describe('tracing', () => { }); expect(transaction).toBeInstanceOf(Transaction); - - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(transaction); + _setSpanForScope(getCurrentScope(), transaction); return transaction; } @@ -207,19 +205,20 @@ describe('tracing', () => { const { traceId } = getCurrentScope().getPropagationContext(); - const request = http.get('http://dogs.are.great/'); + // Needs an outer span, or else we skip it + const request = startSpan({ name: 'outer' }, () => http.get('http://dogs.are.great/')); const sentryTraceHeader = request.getHeader('sentry-trace') as string; const baggageHeader = request.getHeader('baggage') as string; const parts = sentryTraceHeader.split('-'); - // Should not include sampling decision since we don't wanna influence the tracing decisions downstream - expect(parts.length).toEqual(2); + expect(parts.length).toEqual(3); expect(parts[0]).toEqual(traceId); expect(parts[1]).toEqual(expect.any(String)); + expect(parts[2]).toEqual('1'); expect(baggageHeader).toEqual( - `sentry-environment=production,sentry-release=1.0.0,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=${traceId}`, + `sentry-environment=production,sentry-release=1.0.0,sentry-public_key=dogsarebadatkeepingsecrets,sentry-trace_id=${traceId},sentry-sample_rate=1,sentry-transaction=outer,sentry-sampled=true`, ); }); @@ -237,7 +236,8 @@ describe('tracing', () => { }, }); - const request = http.get('http://dogs.are.great/'); + // Needs an outer span, or else we skip it + const request = startSpan({ name: 'outer' }, () => http.get('http://dogs.are.great/')); const sentryTraceHeader = request.getHeader('sentry-trace') as string; const baggageHeader = request.getHeader('baggage') as string; @@ -351,8 +351,7 @@ describe('tracing', () => { function createTransactionAndPutOnScope() { addTracingExtensions(); const transaction = startInactiveSpan({ name: 'dogpark' }); - // eslint-disable-next-line deprecation/deprecation - getCurrentScope().setSpan(transaction); + _setSpanForScope(getCurrentScope(), transaction); return transaction; } From bb56bc370eeb07064a4ea58a93b2c9356f063ccd Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Tue, 12 Mar 2024 15:14:50 +0000 Subject: [PATCH 2/2] fix test --- packages/core/src/tracing/dynamicSamplingContext.ts | 12 ++++++------ packages/node/test/integrations/http.test.ts | 10 ++++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts index c0298d011568..04510683a1b9 100644 --- a/packages/core/src/tracing/dynamicSamplingContext.ts +++ b/packages/core/src/tracing/dynamicSamplingContext.ts @@ -49,15 +49,15 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly { }); afterEach(() => { - _setSpanForScope(getCurrentScope(), undefined); + // eslint-disable-next-line deprecation/deprecation + getCurrentScope().setSpan(undefined); }); function createTransactionOnScope( @@ -55,7 +55,8 @@ describe('tracing', () => { }); expect(transaction).toBeInstanceOf(Transaction); - _setSpanForScope(getCurrentScope(), transaction); + // eslint-disable-next-line deprecation/deprecation + getCurrentScope().setSpan(transaction); return transaction; } @@ -351,7 +352,8 @@ describe('tracing', () => { function createTransactionAndPutOnScope() { addTracingExtensions(); const transaction = startInactiveSpan({ name: 'dogpark' }); - _setSpanForScope(getCurrentScope(), transaction); + // eslint-disable-next-line deprecation/deprecation + getCurrentScope().setSpan(transaction); return transaction; }