From da8e9b61bbfa9a37166ac6186a0e3d875b9954c3 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 5 Feb 2024 10:16:45 -0500 Subject: [PATCH] Revert "feat(core): Add metric summaries to spans (#10432)" This reverts commit 94cdd8bda146455b7b4a46ed52a233b9394bd953. --- .../tracing/metric-summaries/scenario.js | 48 ---------- .../suites/tracing/metric-summaries/test.ts | 69 --------------- packages/core/src/metrics/aggregator.ts | 11 +-- .../core/src/metrics/browser-aggregator.ts | 27 +++--- packages/core/src/metrics/metric-summary.ts | 87 ------------------- packages/core/src/tracing/span.ts | 2 - packages/core/src/tracing/transaction.ts | 2 - packages/types/src/event.ts | 3 +- packages/types/src/index.ts | 7 +- packages/types/src/span.ts | 9 -- 10 files changed, 15 insertions(+), 250 deletions(-) delete mode 100644 dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js delete mode 100644 dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts delete mode 100644 packages/core/src/metrics/metric-summary.ts diff --git a/dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js b/dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js deleted file mode 100644 index 8a7dbabe0dec..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js +++ /dev/null @@ -1,48 +0,0 @@ -const { loggingTransport } = require('@sentry-internal/node-integration-tests'); -const Sentry = require('@sentry/node'); - -Sentry.init({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - release: '1.0', - tracesSampleRate: 1.0, - transport: loggingTransport, - _experiments: { - metricsAggregator: true, - }, -}); - -// Stop the process from exiting before the transaction is sent -setInterval(() => {}, 1000); - -Sentry.startSpan( - { - name: 'Test Transaction', - op: 'transaction', - }, - () => { - Sentry.metrics.increment('root-counter'); - Sentry.metrics.increment('root-counter'); - - Sentry.startSpan( - { - name: 'Some other span', - op: 'transaction', - }, - () => { - Sentry.metrics.increment('root-counter'); - Sentry.metrics.increment('root-counter'); - Sentry.metrics.increment('root-counter', 2); - - Sentry.metrics.set('root-set', 'some-value'); - Sentry.metrics.set('root-set', 'another-value'); - Sentry.metrics.set('root-set', 'another-value'); - - Sentry.metrics.gauge('root-gauge', 42); - Sentry.metrics.gauge('root-gauge', 20); - - Sentry.metrics.distribution('root-distribution', 42); - Sentry.metrics.distribution('root-distribution', 20); - }, - ); - }, -); diff --git a/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts b/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts deleted file mode 100644 index 98ed58a75c57..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts +++ /dev/null @@ -1,69 +0,0 @@ -import { createRunner } from '../../../utils/runner'; - -const EXPECTED_TRANSACTION = { - transaction: 'Test Transaction', - _metrics_summary: { - 'c:root-counter@none': { - min: 1, - max: 1, - count: 2, - sum: 2, - tags: { - release: '1.0', - transaction: 'Test Transaction', - }, - }, - }, - spans: expect.arrayContaining([ - expect.objectContaining({ - description: 'Some other span', - op: 'transaction', - _metrics_summary: { - 'c:root-counter@none': { - min: 1, - max: 2, - count: 3, - sum: 4, - tags: { - release: '1.0', - transaction: 'Test Transaction', - }, - }, - 's:root-set@none': { - min: 0, - max: 1, - count: 3, - sum: 2, - tags: { - release: '1.0', - transaction: 'Test Transaction', - }, - }, - 'g:root-gauge@none': { - min: 20, - max: 42, - count: 2, - sum: 62, - tags: { - release: '1.0', - transaction: 'Test Transaction', - }, - }, - 'd:root-distribution@none': { - min: 20, - max: 42, - count: 2, - sum: 62, - tags: { - release: '1.0', - transaction: 'Test Transaction', - }, - }, - }, - }), - ]), -}; - -test('Should add metric summaries to spans', done => { - createRunner(__dirname, 'scenario.js').expect({ transaction: EXPECTED_TRANSACTION }).start(done); -}); diff --git a/packages/core/src/metrics/aggregator.ts b/packages/core/src/metrics/aggregator.ts index 2b331082ab3e..6a49fda5918b 100644 --- a/packages/core/src/metrics/aggregator.ts +++ b/packages/core/src/metrics/aggregator.ts @@ -6,9 +6,8 @@ import type { Primitive, } from '@sentry/types'; import { timestampInSeconds } from '@sentry/utils'; -import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants'; +import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX } from './constants'; import { METRIC_MAP } from './instance'; -import { updateMetricSummaryOnActiveSpan } from './metric-summary'; import type { MetricBucket, MetricType } from './types'; import { getBucketKey, sanitizeTags } from './utils'; @@ -63,11 +62,7 @@ export class MetricsAggregator implements MetricsAggregatorBase { const tags = sanitizeTags(unsanitizedTags); const bucketKey = getBucketKey(metricType, name, unit, tags); - let bucketItem = this._buckets.get(bucketKey); - // If this is a set metric, we need to calculate the delta from the previous weight. - const previousWeight = bucketItem && metricType === SET_METRIC_TYPE ? bucketItem.metric.weight : 0; - if (bucketItem) { bucketItem.metric.add(value); // TODO(abhi): Do we need this check? @@ -87,10 +82,6 @@ export class MetricsAggregator implements MetricsAggregatorBase { this._buckets.set(bucketKey, bucketItem); } - // If value is a string, it's a set metric so calculate the delta from the previous weight. - const val = typeof value === 'string' ? bucketItem.metric.weight - previousWeight : value; - updateMetricSummaryOnActiveSpan(metricType, name, val, unit, unsanitizedTags, bucketKey); - // We need to keep track of the total weight of the buckets so that we can // flush them when we exceed the max weight. this._bucketsTotalWeight += bucketItem.metric.weight; diff --git a/packages/core/src/metrics/browser-aggregator.ts b/packages/core/src/metrics/browser-aggregator.ts index 40cfa1d404ab..5b5c81353024 100644 --- a/packages/core/src/metrics/browser-aggregator.ts +++ b/packages/core/src/metrics/browser-aggregator.ts @@ -1,8 +1,14 @@ -import type { Client, ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types'; +import type { + Client, + ClientOptions, + MeasurementUnit, + MetricBucketItem, + MetricsAggregator, + Primitive, +} from '@sentry/types'; import { timestampInSeconds } from '@sentry/utils'; -import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants'; +import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX } from './constants'; import { METRIC_MAP } from './instance'; -import { updateMetricSummaryOnActiveSpan } from './metric-summary'; import type { MetricBucket, MetricType } from './types'; import { getBucketKey, sanitizeTags } from './utils'; @@ -40,11 +46,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator { const tags = sanitizeTags(unsanitizedTags); const bucketKey = getBucketKey(metricType, name, unit, tags); - - let bucketItem = this._buckets.get(bucketKey); - // If this is a set metric, we need to calculate the delta from the previous weight. - const previousWeight = bucketItem && metricType === SET_METRIC_TYPE ? bucketItem.metric.weight : 0; - + const bucketItem: MetricBucketItem | undefined = this._buckets.get(bucketKey); if (bucketItem) { bucketItem.metric.add(value); // TODO(abhi): Do we need this check? @@ -52,7 +54,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator { bucketItem.timestamp = timestamp; } } else { - bucketItem = { + this._buckets.set(bucketKey, { // @ts-expect-error we don't need to narrow down the type of value here, saves bundle size. metric: new METRIC_MAP[metricType](value), timestamp, @@ -60,13 +62,8 @@ export class BrowserMetricsAggregator implements MetricsAggregator { name, unit, tags, - }; - this._buckets.set(bucketKey, bucketItem); + }); } - - // If value is a string, it's a set metric so calculate the delta from the previous weight. - const val = typeof value === 'string' ? bucketItem.metric.weight - previousWeight : value; - updateMetricSummaryOnActiveSpan(metricType, name, val, unit, unsanitizedTags, bucketKey); } /** diff --git a/packages/core/src/metrics/metric-summary.ts b/packages/core/src/metrics/metric-summary.ts deleted file mode 100644 index dff610574b2c..000000000000 --- a/packages/core/src/metrics/metric-summary.ts +++ /dev/null @@ -1,87 +0,0 @@ -import type { MeasurementUnit, Span } from '@sentry/types'; -import type { MetricSummary } from '@sentry/types'; -import type { Primitive } from '@sentry/types'; -import { dropUndefinedKeys } from '@sentry/utils'; -import { getActiveSpan } from '../tracing'; -import type { MetricType } from './types'; - -/** - * key: bucketKey - * value: [exportKey, MetricSummary] - */ -type MetricSummaryStorage = Map; - -let SPAN_METRIC_SUMMARY: WeakMap | undefined; - -function getMetricStorageForSpan(span: Span): MetricSummaryStorage | undefined { - return SPAN_METRIC_SUMMARY ? SPAN_METRIC_SUMMARY.get(span) : undefined; -} - -/** - * Fetches the metric summary if it exists for the passed span - */ -export function getMetricSummaryJsonForSpan(span: Span): Record | undefined { - const storage = getMetricStorageForSpan(span); - - if (!storage) { - return undefined; - } - const output: Record = {}; - - for (const [, [exportKey, summary]] of storage) { - output[exportKey] = dropUndefinedKeys(summary); - } - - return output; -} - -/** - * Updates the metric summary on the currently active span - */ -export function updateMetricSummaryOnActiveSpan( - metricType: MetricType, - sanitizedName: string, - value: number, - unit: MeasurementUnit, - tags: Record, - bucketKey: string, -): void { - const span = getActiveSpan(); - if (span) { - const storage = getMetricStorageForSpan(span) || new Map(); - - const exportKey = `${metricType}:${sanitizedName}@${unit}`; - const bucketItem = storage.get(bucketKey); - - if (bucketItem) { - const [, summary] = bucketItem; - storage.set(bucketKey, [ - exportKey, - { - min: Math.min(summary.min, value), - max: Math.max(summary.max, value), - count: (summary.count += 1), - sum: (summary.sum += value), - tags: summary.tags, - }, - ]); - } else { - storage.set(bucketKey, [ - exportKey, - { - min: value, - max: value, - count: 1, - sum: value, - tags, - }, - ]); - } - - if (!SPAN_METRIC_SUMMARY) { - SPAN_METRIC_SUMMARY = new WeakMap(); - } - - SPAN_METRIC_SUMMARY.set(span, storage); - } -} diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts index 1e12628ae2a1..165677455d7f 100644 --- a/packages/core/src/tracing/span.ts +++ b/packages/core/src/tracing/span.ts @@ -16,7 +16,6 @@ import type { import { dropUndefinedKeys, logger, timestampInSeconds, uuid4 } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; -import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../semanticAttributes'; import { getRootSpan } from '../utils/getRootSpan'; import { @@ -625,7 +624,6 @@ export class Span implements SpanInterface { timestamp: this._endTime, trace_id: this._traceId, origin: this._attributes[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN] as SpanOrigin | undefined, - _metrics_summary: getMetricSummaryJsonForSpan(this), }); } diff --git a/packages/core/src/tracing/transaction.ts b/packages/core/src/tracing/transaction.ts index 709aa628f42e..026723929471 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -15,7 +15,6 @@ import { dropUndefinedKeys, logger } from '@sentry/utils'; import { DEBUG_BUILD } from '../debug-build'; import type { Hub } from '../hub'; import { getCurrentHub } from '../hub'; -import { getMetricSummaryJsonForSpan } from '../metrics/metric-summary'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes'; import { spanTimeInputToSeconds, spanToJSON, spanToTraceContext } from '../utils/spanUtils'; import { getDynamicSamplingContextFromSpan } from './dynamicSamplingContext'; @@ -332,7 +331,6 @@ export class Transaction extends SpanClass implements TransactionInterface { capturedSpanIsolationScope, dynamicSamplingContext: getDynamicSamplingContextFromSpan(this), }, - _metrics_summary: getMetricSummaryJsonForSpan(this), ...(source && { transaction_info: { source, diff --git a/packages/types/src/event.ts b/packages/types/src/event.ts index 9e16100bbb1b..50322f18fbc6 100644 --- a/packages/types/src/event.ts +++ b/packages/types/src/event.ts @@ -11,7 +11,7 @@ import type { Request } from './request'; import type { CaptureContext } from './scope'; import type { SdkInfo } from './sdkinfo'; import type { Severity, SeverityLevel } from './severity'; -import type { MetricSummary, Span, SpanJSON } from './span'; +import type { Span, SpanJSON } from './span'; import type { Thread } from './thread'; import type { TransactionSource } from './transaction'; import type { User } from './user'; @@ -73,7 +73,6 @@ export interface ErrorEvent extends Event { } export interface TransactionEvent extends Event { type: 'transaction'; - _metrics_summary?: Record; } /** JSDoc */ diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index d4fcd439ae4a..5970383febc3 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -99,7 +99,6 @@ export type { SpanJSON, SpanContextData, TraceFlag, - MetricSummary, } from './span'; export type { StackFrame } from './stackframe'; export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace'; @@ -151,9 +150,5 @@ export type { export type { BrowserClientReplayOptions, BrowserClientProfilingOptions } from './browseroptions'; export type { CheckIn, MonitorConfig, FinishedCheckIn, InProgressCheckIn, SerializedCheckIn } from './checkin'; -export type { - MetricsAggregator, - MetricBucketItem, - MetricInstance, -} from './metrics'; +export type { MetricsAggregator, MetricBucketItem, MetricInstance } from './metrics'; export type { ParameterizedString } from './parameterize'; diff --git a/packages/types/src/span.ts b/packages/types/src/span.ts index 0743497f1411..73c2fbdaaaa8 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -31,14 +31,6 @@ export type SpanAttributes = Partial<{ }> & Record; -export type MetricSummary = { - min: number; - max: number; - count: number; - sum: number; - tags?: Record | undefined; -}; - /** This type is aligned with the OpenTelemetry TimeInput type. */ export type SpanTimeInput = HrTime | number | Date; @@ -55,7 +47,6 @@ export interface SpanJSON { timestamp?: number; trace_id: string; origin?: SpanOrigin; - _metrics_summary?: Record; } // These are aligned with OpenTelemetry trace flags