From 18aa8e6dca962bbcd8a0f3d3dd118e9b9b6d0c15 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 9 Apr 2024 13:28:07 +0200 Subject: [PATCH 1/2] map statsd to metric_bucket --- packages/core/src/transports/base.ts | 6 +++--- packages/types/src/datacategory.ts | 6 +++--- packages/types/src/envelope.ts | 2 ++ packages/utils/src/envelope.ts | 5 ++--- packages/utils/src/ratelimit.ts | 12 +++++++----- 5 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/core/src/transports/base.ts b/packages/core/src/transports/base.ts index b942266457f9..0ad22d3d7a15 100644 --- a/packages/core/src/transports/base.ts +++ b/packages/core/src/transports/base.ts @@ -49,10 +49,10 @@ export function createTransport( // Drop rate limited items from envelope forEachEnvelopeItem(envelope, (item, type) => { - const envelopeItemDataCategory = envelopeItemTypeToDataCategory(type); - if (isRateLimited(rateLimits, envelopeItemDataCategory)) { + const dataCategory = envelopeItemTypeToDataCategory(type); + if (isRateLimited(rateLimits, dataCategory)) { const event: Event | undefined = getEventForEnvelopeItem(item, type); - options.recordDroppedEvent('ratelimit_backoff', envelopeItemDataCategory, event); + options.recordDroppedEvent('ratelimit_backoff', dataCategory, event); } else { filteredEnvelopeItems.push(item); } diff --git a/packages/types/src/datacategory.ts b/packages/types/src/datacategory.ts index de482bc3d7cc..bd1c0b693e4d 100644 --- a/packages/types/src/datacategory.ts +++ b/packages/types/src/datacategory.ts @@ -1,7 +1,7 @@ // This type is used in various places like Client Reports and Rate Limit Categories // See: // - https://develop.sentry.dev/sdk/rate-limiting/#definitions -// - https://github.com/getsentry/relay/blob/c3b339e151c1e548ede489a01c65db82472c8751/relay-common/src/constants.rs#L139-L152 +// - https://github.com/getsentry/relay/blob/ec791fed9c2260688f25ea6a6d53ab913927e9a5/relay-base-schema/src/data_category.rs#L91 // - https://develop.sentry.dev/sdk/client-reports/#envelope-item-payload under `discarded_events` export type DataCategory = // Reserved and only used in edgecases, unlikely to be ever actually used @@ -26,8 +26,8 @@ export type DataCategory = | 'monitor' // Feedback type event (v2) | 'feedback' - // Statsd type event for metrics - | 'statsd' + // Metrics sent via the statsd or metrics envelope items + | 'metric_bucket' // Span | 'span' // Unknown data category diff --git a/packages/types/src/envelope.ts b/packages/types/src/envelope.ts index 9c5c045a4648..c72406b4539f 100644 --- a/packages/types/src/envelope.ts +++ b/packages/types/src/envelope.ts @@ -24,6 +24,8 @@ export type DynamicSamplingContext = { sampled?: string; }; +// https://github.com/getsentry/relay/blob/311b237cd4471042352fa45e7a0824b8995f216f/relay-server/src/envelope.rs#L154 +// https://develop.sentry.dev/sdk/envelopes/#data-model export type EnvelopeItemType = | 'client_report' | 'user_report' diff --git a/packages/utils/src/envelope.ts b/packages/utils/src/envelope.ts index 4c6a6eea724d..8b1345b112d7 100644 --- a/packages/utils/src/envelope.ts +++ b/packages/utils/src/envelope.ts @@ -209,8 +209,7 @@ const ITEM_TYPE_TO_DATA_CATEGORY_MAP: Record = { check_in: 'monitor', feedback: 'feedback', span: 'span', - // TODO: This is a temporary workaround until we have a proper data category for metrics - statsd: 'unknown', + statsd: 'metric_bucket', }; /** @@ -220,7 +219,7 @@ export function envelopeItemTypeToDataCategory(type: EnvelopeItemType): DataCate return ITEM_TYPE_TO_DATA_CATEGORY_MAP[type]; } -/** Extracts the minimal SDK info from from the metadata or an events */ +/** Extracts the minimal SDK info from the metadata or an events */ export function getSdkMetadataForEnvelopeHeader(metadataOrEvent?: SdkMetadata | Event): SdkInfo | undefined { if (!metadataOrEvent || !metadataOrEvent.sdk) { return; diff --git a/packages/utils/src/ratelimit.ts b/packages/utils/src/ratelimit.ts index c3353c8034a6..eda9375a9de6 100644 --- a/packages/utils/src/ratelimit.ts +++ b/packages/utils/src/ratelimit.ts @@ -1,4 +1,4 @@ -import type { TransportMakeRequestResponse } from '@sentry/types'; +import type { DataCategory, TransportMakeRequestResponse } from '@sentry/types'; // Intentionally keeping the key broad, as we don't know for sure what rate limit headers get returned from backend export type RateLimits = Record; @@ -32,15 +32,15 @@ export function parseRetryAfterHeader(header: string, now: number = Date.now()): * * @return the time in ms that the category is disabled until or 0 if there's no active rate limit. */ -export function disabledUntil(limits: RateLimits, category: string): number { - return limits[category] || limits.all || 0; +export function disabledUntil(limits: RateLimits, dataCategory: DataCategory): number { + return limits[dataCategory] || limits.all || 0; } /** * Checks if a category is rate limited */ -export function isRateLimited(limits: RateLimits, category: string, now: number = Date.now()): boolean { - return disabledUntil(limits, category) > now; +export function isRateLimited(limits: RateLimits, dataCategory: DataCategory, now: number = Date.now()): boolean { + return disabledUntil(limits, dataCategory) > now; } /** @@ -74,6 +74,8 @@ export function updateRateLimits( * ;;... * is what's being limited (org, project, or key) - ignored by SDK * is an arbitrary string like "org_quota" - ignored by SDK + * Semicolon-separated list of metric namespace identifiers. Defines which namespace(s) will be affected. + * Only present if rate limit applies to the metric_bucket data category. */ for (const limit of rateLimitHeader.trim().split(',')) { const [retryAfter, categories] = limit.split(':', 2); From bc7be91e198009bc3b99c33d6ff068642a229fb0 Mon Sep 17 00:00:00 2001 From: s1gr1d Date: Tue, 9 Apr 2024 14:13:35 +0200 Subject: [PATCH 2/2] add metrics rate limits --- packages/utils/src/ratelimit.ts | 13 +++++++-- packages/utils/test/ratelimit.test.ts | 40 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-) diff --git a/packages/utils/src/ratelimit.ts b/packages/utils/src/ratelimit.ts index eda9375a9de6..77be8cf7fa9e 100644 --- a/packages/utils/src/ratelimit.ts +++ b/packages/utils/src/ratelimit.ts @@ -67,7 +67,7 @@ export function updateRateLimits( * rate limit headers are of the form *
,
,.. * where each
is of the form - * : : : + * : : : : * where * is a delay in seconds * is the event type(s) (error, transaction, etc) being rate limited and is of the form @@ -78,14 +78,21 @@ export function updateRateLimits( * Only present if rate limit applies to the metric_bucket data category. */ for (const limit of rateLimitHeader.trim().split(',')) { - const [retryAfter, categories] = limit.split(':', 2); + const [retryAfter, categories, , , namespaces] = limit.split(':', 5); const headerDelay = parseInt(retryAfter, 10); const delay = (!isNaN(headerDelay) ? headerDelay : 60) * 1000; // 60sec default if (!categories) { updatedRateLimits.all = now + delay; } else { for (const category of categories.split(';')) { - updatedRateLimits[category] = now + delay; + if (category === 'metric_bucket') { + // namespaces will be present when category === 'metric_bucket' + if (!namespaces || namespaces.split(';').includes('custom')) { + updatedRateLimits[category] = now + delay; + } + } else { + updatedRateLimits[category] = now + delay; + } } } } diff --git a/packages/utils/test/ratelimit.test.ts b/packages/utils/test/ratelimit.test.ts index e9447aefcc73..5833548727f7 100644 --- a/packages/utils/test/ratelimit.test.ts +++ b/packages/utils/test/ratelimit.test.ts @@ -197,3 +197,43 @@ describe('updateRateLimits()', () => { expect(updatedRateLimits.all).toEqual(60_000); }); }); + +describe('data category "metric_bucket"', () => { + test('should add limit for `metric_bucket` category when namespaces contain "custom"', () => { + const rateLimits: RateLimits = {}; + const headers = { + 'retry-after': null, + 'x-sentry-rate-limits': '42:metric_bucket:::custom', + }; + const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0); + expect(updatedRateLimits.metric_bucket).toEqual(42 * 1000); + }); + + test('should not add limit for `metric_bucket` category when namespaces do not contain "custom"', () => { + const rateLimits: RateLimits = {}; + const headers = { + 'retry-after': null, + 'x-sentry-rate-limits': '42:metric_bucket:::namespace1;namespace2', + }; + const updatedRateLimits = updateRateLimits(rateLimits, { headers }, 0); + expect(updatedRateLimits.metric_bucket).toBeUndefined(); + }); + + test('should add limit for `metric_bucket` category when namespaces are empty', () => { + const rateLimits: RateLimits = {}; + + const headers1 = { + 'retry-after': null, + 'x-sentry-rate-limits': '42:metric_bucket', // without semicolon at the end + }; + const updatedRateLimits1 = updateRateLimits(rateLimits, { headers: headers1 }, 0); + expect(updatedRateLimits1.metric_bucket).toEqual(42 * 1000); + + const headers2 = { + 'retry-after': null, + 'x-sentry-rate-limits': '42:metric_bucket:organization:quota_exceeded:', // with semicolon at the end + }; + const updatedRateLimits2 = updateRateLimits(rateLimits, { headers: headers2 }, 0); + expect(updatedRateLimits2.metric_bucket).toEqual(42 * 1000); + }); +});