From c7c6bb29e1a74b80e3c53048fc92474cfb268754 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 7 Mar 2024 15:53:25 +0000 Subject: [PATCH 1/2] ref(browser): Store browser metrics as attributes instead of tags (#10823) Extracted this out from https://github.com/getsentry/sentry-javascript/pull/10808, as this is a bit more impactful, possibly. --- .../tracing/metrics/handlers-lcp/test.ts | 6 +-- .../tracing/metrics/web-vitals-cls/test.ts | 6 +-- .../tracing/metrics/web-vitals-lcp/test.ts | 13 ++++--- .../src/browser/metrics/index.ts | 37 +++++-------------- 4 files changed, 22 insertions(+), 40 deletions(-) diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts index 23cd29099a0f..a2801f4e4016 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/handlers-lcp/test.ts @@ -29,9 +29,9 @@ sentryTest( expect(eventData.measurements).toBeDefined(); expect(eventData.measurements?.lcp?.value).toBeDefined(); - expect(eventData.tags?.['lcp.element']).toBe('body > img'); - expect(eventData.tags?.['lcp.size']).toBe(107400); - expect(eventData.tags?.['lcp.url']).toBe('https://example.com/path/to/image.png'); + expect(eventData.contexts?.trace?.data?.['lcp.element']).toBe('body > img'); + expect(eventData.contexts?.trace?.data?.['lcp.size']).toBe(107400); + expect(eventData.contexts?.trace?.data?.['lcp.url']).toBe('https://example.com/path/to/image.png'); const lcp = await (await page.waitForFunction('window._LCP')).jsonValue(); const lcp2 = await (await page.waitForFunction('window._LCP2')).jsonValue(); diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls/test.ts index 0dee366c75f4..00fc906aa60e 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-cls/test.ts @@ -23,7 +23,7 @@ sentryTest('should capture a "GOOD" CLS vital with its source(s).', async ({ get expect(eventData.measurements?.cls?.value).toBeGreaterThan(0.03); expect(eventData.measurements?.cls?.value).toBeLessThan(0.07); - expect(eventData.tags?.['cls.source.1']).toBe('body > div#content > p#partial'); + expect(eventData.contexts?.trace?.data?.['cls.source.1']).toBe('body > div#content > p#partial'); }); sentryTest('should capture a "MEH" CLS vital with its source(s).', async ({ getLocalTestPath, page }) => { @@ -37,7 +37,7 @@ sentryTest('should capture a "MEH" CLS vital with its source(s).', async ({ getL expect(eventData.measurements?.cls?.value).toBeGreaterThan(0.18); expect(eventData.measurements?.cls?.value).toBeLessThan(0.23); - expect(eventData.tags?.['cls.source.1']).toBe('body > div#content > p'); + expect(eventData.contexts?.trace?.data?.['cls.source.1']).toBe('body > div#content > p'); }); sentryTest('should capture a "POOR" CLS vital with its source(s).', async ({ getLocalTestPath, page }) => { @@ -50,5 +50,5 @@ sentryTest('should capture a "POOR" CLS vital with its source(s).', async ({ get // Flakey value dependent on timings -> we check for a range expect(eventData.measurements?.cls?.value).toBeGreaterThan(0.34); expect(eventData.measurements?.cls?.value).toBeLessThan(0.36); - expect(eventData.tags?.['cls.source.1']).toBe('body > div#content > p'); + expect(eventData.contexts?.trace?.data?.['cls.source.1']).toBe('body > div#content > p'); }); diff --git a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts index cbe60ae1ea62..2cfcbe58806e 100644 --- a/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts +++ b/dev-packages/browser-integration-tests/suites/tracing/metrics/web-vitals-lcp/test.ts @@ -10,9 +10,10 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN sentryTest.skip(); } - await page.route('**/path/to/image.png', (route: Route) => - route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` }), - ); + page.route('**', route => route.continue()); + page.route('**/path/to/image.png', async (route: Route) => { + return route.fulfill({ path: `${__dirname}/assets/sentry-logo-600x179.png` }); + }); const url = await getLocalTestPath({ testDir: __dirname }); const [eventData] = await Promise.all([ @@ -24,7 +25,7 @@ sentryTest('should capture a LCP vital with element details.', async ({ browserN expect(eventData.measurements).toBeDefined(); expect(eventData.measurements?.lcp?.value).toBeDefined(); - expect(eventData.tags?.['lcp.element']).toBe('body > img'); - expect(eventData.tags?.['lcp.size']).toBe(107400); - expect(eventData.tags?.['lcp.url']).toBe('https://example.com/path/to/image.png'); + expect(eventData.contexts?.trace?.data?.['lcp.element']).toBe('body > img'); + expect(eventData.contexts?.trace?.data?.['lcp.size']).toBe(107400); + expect(eventData.contexts?.trace?.data?.['lcp.url']).toBe('https://example.com/path/to/image.png'); }); diff --git a/packages/tracing-internal/src/browser/metrics/index.ts b/packages/tracing-internal/src/browser/metrics/index.ts index 6a25bd00c92f..d7237f05f8f6 100644 --- a/packages/tracing-internal/src/browser/metrics/index.ts +++ b/packages/tracing-internal/src/browser/metrics/index.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines */ -import type { SentrySpan } from '@sentry/core'; import { getActiveSpan, startInactiveSpan } from '@sentry/core'; import { setMeasurement } from '@sentry/core'; import type { Measurements, Span, StartSpanOptions } from '@sentry/types'; @@ -445,15 +444,11 @@ function _trackNavigator(span: Span): void { const connection = navigator.connection; if (connection) { if (connection.effectiveType) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('effectiveConnectionType', connection.effectiveType); + span.setAttribute('effectiveConnectionType', connection.effectiveType); } if (connection.type) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('connectionType', connection.type); + span.setAttribute('connectionType', connection.type); } if (isMeasurementValue(connection.rtt)) { @@ -462,15 +457,11 @@ function _trackNavigator(span: Span): void { } if (isMeasurementValue(navigator.deviceMemory)) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('deviceMemory', `${navigator.deviceMemory} GB`); + span.setAttribute('deviceMemory', `${navigator.deviceMemory} GB`); } if (isMeasurementValue(navigator.hardwareConcurrency)) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('hardwareConcurrency', String(navigator.hardwareConcurrency)); + span.setAttribute('hardwareConcurrency', String(navigator.hardwareConcurrency)); } } @@ -482,36 +473,26 @@ function _tagMetricInfo(span: Span): void { // Capture Properties of the LCP element that contributes to the LCP. if (_lcpEntry.element) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('lcp.element', htmlTreeAsString(_lcpEntry.element)); + span.setAttribute('lcp.element', htmlTreeAsString(_lcpEntry.element)); } if (_lcpEntry.id) { - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('lcp.id', _lcpEntry.id); + span.setAttribute('lcp.id', _lcpEntry.id); } if (_lcpEntry.url) { // Trim URL to the first 200 characters. - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('lcp.url', _lcpEntry.url.trim().slice(0, 200)); + span.setAttribute('lcp.url', _lcpEntry.url.trim().slice(0, 200)); } - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag('lcp.size', _lcpEntry.size); + span.setAttribute('lcp.size', _lcpEntry.size); } // See: https://developer.mozilla.org/en-US/docs/Web/API/LayoutShift if (_clsEntry && _clsEntry.sources) { DEBUG_BUILD && logger.log('[Measurements] Adding CLS Data'); _clsEntry.sources.forEach((source, index) => - // TODO: Can we rewrite this to an attribute? - // eslint-disable-next-line deprecation/deprecation - (span as SentrySpan).setTag(`cls.source.${index + 1}`, htmlTreeAsString(source.node)), + span.setAttribute(`cls.source.${index + 1}`, htmlTreeAsString(source.node)), ); } } From 4b261ce0a5a834ac206652c8fbffa8f6ca64d00f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 7 Mar 2024 17:42:09 +0000 Subject: [PATCH 2/2] feat: Remove tags from spans & transactions (#10809) We've refactored all usage away, now we can actually get rid of the tags on the span themselves. --- packages/core/src/tracing/sentrySpan.ts | 28 ------------------- packages/core/src/tracing/transaction.ts | 2 -- packages/core/src/utils/spanUtils.ts | 3 +- .../profiling-node/test/hubextensions.test.ts | 5 ---- packages/types/src/span.ts | 7 ----- packages/types/src/transaction.ts | 8 +----- 6 files changed, 2 insertions(+), 51 deletions(-) diff --git a/packages/core/src/tracing/sentrySpan.ts b/packages/core/src/tracing/sentrySpan.ts index eed2f4894710..d4da47231c51 100644 --- a/packages/core/src/tracing/sentrySpan.ts +++ b/packages/core/src/tracing/sentrySpan.ts @@ -1,5 +1,4 @@ import type { - Primitive, Span, SpanAttributeValue, SpanAttributes, @@ -65,12 +64,6 @@ export class SpanRecorder { * Span contains all data about a span */ export class SentrySpan implements Span { - /** - * Tags for the span. - * @deprecated Use `spanToJSON(span).atttributes` instead. - */ - public tags: { [key: string]: Primitive }; - /** * Data for the span. * @deprecated Use `spanToJSON(span).atttributes` instead. @@ -117,8 +110,6 @@ export class SentrySpan implements Span { this._spanId = spanContext.spanId || uuid4().substring(16); this._startTime = spanContext.startTimestamp || timestampInSeconds(); // eslint-disable-next-line deprecation/deprecation - this.tags = spanContext.tags ? { ...spanContext.tags } : {}; - // eslint-disable-next-line deprecation/deprecation this.data = spanContext.data ? { ...spanContext.data } : {}; this._attributes = {}; @@ -328,21 +319,6 @@ export class SentrySpan implements Span { return childSpan; } - /** - * Sets the tag attribute on the current span. - * - * Can also be used to unset a tag, by passing `undefined`. - * - * @param key Tag key - * @param value Tag value - * @deprecated Use `setAttribute()` instead. - */ - public setTag(key: string, value: Primitive): this { - // eslint-disable-next-line deprecation/deprecation - this.tags = { ...this.tags, [key]: value }; - return this; - } - /** * Sets the data attribute on the current span * @param key Data key @@ -439,8 +415,6 @@ export class SentrySpan implements Span { spanId: this._spanId, startTimestamp: this._startTime, status: this._status, - // eslint-disable-next-line deprecation/deprecation - tags: this.tags, traceId: this._traceId, }); } @@ -471,8 +445,6 @@ export class SentrySpan implements Span { span_id: this._spanId, start_timestamp: this._startTime, status: getStatusMessage(this._status), - // eslint-disable-next-line deprecation/deprecation - tags: Object.keys(this.tags).length > 0 ? this.tags : undefined, timestamp: this._endTime, trace_id: this._traceId, origin: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined, diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index ad7a120a52a6..31d50253d2e0 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -286,8 +286,6 @@ export class Transaction extends SentrySpan implements TransactionInterface { }, spans, start_timestamp: this._startTime, - // eslint-disable-next-line deprecation/deprecation - tags: this.tags, timestamp: this._endTime, transaction: this._name, type: 'transaction', diff --git a/packages/core/src/utils/spanUtils.ts b/packages/core/src/utils/spanUtils.ts index e6f50edcfd8a..a46503259b01 100644 --- a/packages/core/src/utils/spanUtils.ts +++ b/packages/core/src/utils/spanUtils.ts @@ -22,7 +22,7 @@ export const TRACE_FLAG_SAMPLED = 0x1; */ export function spanToTraceContext(span: Span): TraceContext { const { spanId: span_id, traceId: trace_id } = span.spanContext(); - const { data, op, parent_span_id, status, tags, origin } = spanToJSON(span); + const { data, op, parent_span_id, status, origin } = spanToJSON(span); return dropUndefinedKeys({ data, @@ -30,7 +30,6 @@ export function spanToTraceContext(span: Span): TraceContext { parent_span_id, span_id, status, - tags, trace_id, origin, }); diff --git a/packages/profiling-node/test/hubextensions.test.ts b/packages/profiling-node/test/hubextensions.test.ts index 362803e4b48d..060892be9d0f 100644 --- a/packages/profiling-node/test/hubextensions.test.ts +++ b/packages/profiling-node/test/hubextensions.test.ts @@ -15,7 +15,6 @@ import { __PRIVATE__wrapStartTransactionWithProfiling } from '../src/hubextensio function makeTransactionMock(options = {}): Transaction { return { metadata: {}, - tags: {}, sampled: true, contexts: {}, startChild: () => ({ end: () => void 0 }), @@ -29,10 +28,6 @@ function makeTransactionMock(options = {}): Transaction { // @ts-expect-error - contexts is private this.contexts[key] = context; }, - setTag(this: Transaction, key: string, value: any) { - // eslint-disable-next-line deprecation/deprecation - this.tags[key] = value; - }, setMetadata(this: Transaction, metadata: Partial) { // eslint-disable-next-line deprecation/deprecation this.metadata = { ...metadata } as TransactionMetadata; diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index ddc3057e6cbf..de925d25c123 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -50,7 +50,6 @@ export interface SpanJSON { span_id: string; start_timestamp: number; status?: string; - tags?: { [key: string]: Primitive }; timestamp?: number; trace_id: string; origin?: SpanOrigin; @@ -132,12 +131,6 @@ export interface SpanContext { */ traceId?: string | undefined; - /** - * Tags of the Span. - * @deprecated Pass `attributes` instead. - */ - tags?: { [key: string]: Primitive }; - /** * Data of the Span. * @deprecated Pass `attributes` instead. diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index 4b05fb430565..2e44730ac808 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -1,7 +1,7 @@ import type { Context } from './context'; import type { DynamicSamplingContext } from './envelope'; import type { MeasurementUnit } from './measurement'; -import type { ExtractedNodeRequestData, Primitive, WorkerLocation } from './misc'; +import type { ExtractedNodeRequestData, WorkerLocation } from './misc'; import type { PolymorphicRequest } from './polymorphics'; import type { Span, SpanAttributes, SpanContext } from './span'; @@ -81,12 +81,6 @@ export interface Transaction extends Omit, Sp */ startTimestamp: number; - /** - * Tags for the transaction. - * @deprecated Use `getSpanAttributes(transaction)` instead. - */ - tags: { [key: string]: Primitive }; - /** * Data for the transaction. * @deprecated Use `getSpanAttributes(transaction)` instead.