From 7b3a22d67648a26e44c4b16dd8f23cf77b3d295a Mon Sep 17 00:00:00 2001 From: Michi Hoffmann Date: Mon, 12 Feb 2024 16:16:16 +0100 Subject: [PATCH] feat(core): Add metric summaries to spans (#10554) Co-authored-by: Abhijeet Prasad --- .../tracing/metric-summaries/scenario.js | 56 ++++++++++++ .../suites/tracing/metric-summaries/test.ts | 91 +++++++++++++++++++ packages/core/src/metrics/aggregator.ts | 11 ++- .../core/src/metrics/browser-aggregator.ts | 27 +++--- packages/core/src/metrics/constants.ts | 2 +- packages/core/src/metrics/metric-summary.ts | 91 +++++++++++++++++++ packages/core/src/metrics/utils.ts | 2 +- 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 ++ 12 files changed, 286 insertions(+), 17 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js create mode 100644 dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts create 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 new file mode 100644 index 000000000000..ef68afb06576 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js @@ -0,0 +1,56 @@ +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', 1, { + tags: { + email: 'jon.doe@example.com', + }, + }); + Sentry.metrics.increment('root-counter', 1, { + tags: { + email: 'jane.doe@example.com', + }, + }); + + 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 new file mode 100644 index 000000000000..94f5fdc30c70 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts @@ -0,0 +1,91 @@ +import { createRunner } from '../../../utils/runner'; + +const EXPECTED_TRANSACTION = { + transaction: 'Test Transaction', + _metrics_summary: { + 'c:root-counter@none': [ + { + min: 1, + max: 1, + count: 1, + sum: 1, + tags: { + release: '1.0', + transaction: 'Test Transaction', + email: 'jon.doe@example.com', + }, + }, + { + min: 1, + max: 1, + count: 1, + sum: 1, + tags: { + release: '1.0', + transaction: 'Test Transaction', + email: 'jane.doe@example.com', + }, + }, + ], + }, + 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 6a49fda5918b..2b331082ab3e 100644 --- a/packages/core/src/metrics/aggregator.ts +++ b/packages/core/src/metrics/aggregator.ts @@ -6,8 +6,9 @@ import type { Primitive, } from '@sentry/types'; import { timestampInSeconds } from '@sentry/utils'; -import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX } from './constants'; +import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants'; import { METRIC_MAP } from './instance'; +import { updateMetricSummaryOnActiveSpan } from './metric-summary'; import type { MetricBucket, MetricType } from './types'; import { getBucketKey, sanitizeTags } from './utils'; @@ -62,7 +63,11 @@ 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? @@ -82,6 +87,10 @@ 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 5b5c81353024..40cfa1d404ab 100644 --- a/packages/core/src/metrics/browser-aggregator.ts +++ b/packages/core/src/metrics/browser-aggregator.ts @@ -1,14 +1,8 @@ -import type { - Client, - ClientOptions, - MeasurementUnit, - MetricBucketItem, - MetricsAggregator, - Primitive, -} from '@sentry/types'; +import type { Client, ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types'; import { timestampInSeconds } from '@sentry/utils'; -import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX } from './constants'; +import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants'; import { METRIC_MAP } from './instance'; +import { updateMetricSummaryOnActiveSpan } from './metric-summary'; import type { MetricBucket, MetricType } from './types'; import { getBucketKey, sanitizeTags } from './utils'; @@ -46,7 +40,11 @@ export class BrowserMetricsAggregator implements MetricsAggregator { const tags = sanitizeTags(unsanitizedTags); const bucketKey = getBucketKey(metricType, name, unit, tags); - const bucketItem: MetricBucketItem | undefined = this._buckets.get(bucketKey); + + 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? @@ -54,7 +52,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator { bucketItem.timestamp = timestamp; } } else { - this._buckets.set(bucketKey, { + bucketItem = { // @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, @@ -62,8 +60,13 @@ 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/constants.ts b/packages/core/src/metrics/constants.ts index e89e0fd1562b..a5f3a87f57d5 100644 --- a/packages/core/src/metrics/constants.ts +++ b/packages/core/src/metrics/constants.ts @@ -21,7 +21,7 @@ export const NAME_AND_TAG_KEY_NORMALIZATION_REGEX = /[^a-zA-Z0-9_/.-]+/g; * * See: https://develop.sentry.dev/sdk/metrics/#normalization */ -export const TAG_VALUE_NORMALIZATION_REGEX = /[^\w\d_:/@.{}[\]$-]+/g; +export const TAG_VALUE_NORMALIZATION_REGEX = /[^\w\d\s_:/@.{}[\]$-]+/g; /** * This does not match spec in https://develop.sentry.dev/sdk/metrics diff --git a/packages/core/src/metrics/metric-summary.ts b/packages/core/src/metrics/metric-summary.ts new file mode 100644 index 000000000000..bf2e828dae1b --- /dev/null +++ b/packages/core/src/metrics/metric-summary.ts @@ -0,0 +1,91 @@ +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) { + if (!output[exportKey]) { + output[exportKey] = []; + } + + output[exportKey].push(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/metrics/utils.ts b/packages/core/src/metrics/utils.ts index a6674bcf30e1..7b1cf96a8462 100644 --- a/packages/core/src/metrics/utils.ts +++ b/packages/core/src/metrics/utils.ts @@ -62,7 +62,7 @@ export function sanitizeTags(unsanitizedTags: Record): Record for (const key in unsanitizedTags) { if (Object.prototype.hasOwnProperty.call(unsanitizedTags, key)) { const sanitizedKey = key.replace(NAME_AND_TAG_KEY_NORMALIZATION_REGEX, '_'); - tags[sanitizedKey] = String(unsanitizedTags[key]).replace(TAG_VALUE_NORMALIZATION_REGEX, '_'); + tags[sanitizedKey] = String(unsanitizedTags[key]).replace(TAG_VALUE_NORMALIZATION_REGEX, ''); } } return tags; diff --git a/packages/core/src/tracing/span.ts b/packages/core/src/tracing/span.ts index 28a2de56475d..1c178fc05fbe 100644 --- a/packages/core/src/tracing/span.ts +++ b/packages/core/src/tracing/span.ts @@ -16,6 +16,7 @@ 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 { @@ -624,6 +625,7 @@ 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 026723929471..709aa628f42e 100644 --- a/packages/core/src/tracing/transaction.ts +++ b/packages/core/src/tracing/transaction.ts @@ -15,6 +15,7 @@ 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'; @@ -331,6 +332,7 @@ 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 50322f18fbc6..15b253c666c8 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 { Span, SpanJSON } from './span'; +import type { MetricSummary, Span, SpanJSON } from './span'; import type { Thread } from './thread'; import type { TransactionSource } from './transaction'; import type { User } from './user'; @@ -73,6 +73,7 @@ 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 5970383febc3..d4fcd439ae4a 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -99,6 +99,7 @@ export type { SpanJSON, SpanContextData, TraceFlag, + MetricSummary, } from './span'; export type { StackFrame } from './stackframe'; export type { Stacktrace, StackParser, StackLineParser, StackLineParserFn } from './stacktrace'; @@ -150,5 +151,9 @@ 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 fc7f1077368e..6aa6ea1113f6 100644 --- a/packages/types/src/span.ts +++ b/packages/types/src/span.ts @@ -31,6 +31,14 @@ 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; @@ -47,6 +55,7 @@ export interface SpanJSON { timestamp?: number; trace_id: string; origin?: SpanOrigin; + _metrics_summary?: Record>; } // These are aligned with OpenTelemetry trace flags