From bdc1dcaef8c1015e08ac0dd6dc6241e3f11103d1 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:45:44 +0200 Subject: [PATCH 1/2] feat(server-utils)!: Start Redis spans as `cache` spans --- MIGRATION.md | 2 + .../suites/tracing/redis-dc/instrument.mjs | 3 +- .../redis-dc/scenario-redis-5-tracing.mjs | 4 + .../suites/tracing/redis-dc/test.ts | 63 +++++++ .../src/integrations/redis/index.ts | 27 +-- .../redis/ioredis-channel-subscriber.ts | 37 ++-- .../src/integrations/redis/redis-cache.ts | 99 ++++++----- .../integrations/redis/redis-dc-subscriber.ts | 27 +-- .../redis/ioredis-channel-subscriber.test.ts | 31 +++- .../integrations/redis/redis-cache.test.ts | 163 ++++++++++++------ 10 files changed, 309 insertions(+), 147 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index 1ccb92ccec49..31271b7d6c69 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -962,6 +962,8 @@ Messaging span names now read ` ` in every integrat Cache keys are unbounded, so they are no longer part of a cache span name. They remain available on the `cache.key` attribute, and every cache span now also carries a `cache.operation` attribute (`get`, `put`, `remove`) — the value the name is built from. That attribute is set in both trace lifecycles. This affects the redis/ioredis cache spans (`cachePrefixes`), the Nuxt and Nitro storage spans, and the dataloader spans. +A Redis command whose key matches `cachePrefixes` now starts as a `cache.*` span instead of being converted from a `db.query` span at response time. `ignoreSpans` is evaluated at span start, so filters can match these spans by their cache op and name. A failed cache command reports as a cache span too, where it previously stayed a `db.query` span. + A dataloader span no longer carries the loader's `name` either (`dataloader.load usersLoader` becomes `cache.get`), because the cache conventions have no slot for it in the name. It is reported on the `db.collection.name` attribute instead — a loader batches one entity type, so it is the closest thing dataloader has to a collection — and that attribute is set in both trace lifecycles. Unnamed loaders do not set it. AWS SQS `SendMessage`, `SendMessageBatch` and `ReceiveMessage`, and SNS `Publish`, are messaging spans (e.g. `queue.publish`) rather than `rpc` ones now. Every other command on those clients, such as `DeleteMessage`, stays `rpc`. Their names follow the messaging conventions too, so the operation comes first (`my-queue receive` becomes `receive my-queue`, `my-topic send` becomes `send my-topic`). A streamed SNS `Publish` to a platform endpoint is named `send`, because the endpoint ARN it used to carry ends in a per-device id (`endpoint/GCM/myapp/ send`). The full ARN remains on `messaging.destination.name`. diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs index fef89b43c532..2f4c45c014ea 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs @@ -2,10 +2,11 @@ import * as Sentry from '@sentry/node'; import { loggingTransport } from '@sentry-internal/node-integration-tests'; Sentry.init({ - traceLifecycle: 'static', + traceLifecycle: process.env.STREAMED === 'true' ? 'stream' : 'static', dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', tracesSampleRate: 1.0, transport: loggingTransport, integrations: [Sentry.redisIntegration({ cachePrefixes: ['dc-cache:'] })], + ignoreSpans: process.env.IGNORE_CACHE_GET === 'true' ? [{ op: 'cache.get' }] : undefined, }); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs index b98a66cdd273..661b37ef9c6d 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/scenario-redis-5-tracing.mjs @@ -21,6 +21,10 @@ async function run() { await redisClient.get('dc-cache:unavailable-data'); await redisClient.mGet(['dc-test-key', 'dc-cache:test-key', 'dc-cache:unavailable-data']); + + // a failing command on a cache key (GET on a list rejects with WRONGTYPE) + await redisClient.lPush('dc-cache:list-key', 'value'); + await redisClient.get('dc-cache:list-key').catch(() => {}); } finally { await redisClient.disconnect(); } diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts index d97db9ae92fe..33997707113f 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts @@ -1,3 +1,4 @@ +import type { SerializedStreamedSpanContainer } from '@sentry/core'; import { afterAll, expect } from 'vitest'; import { cleanupChildProcesses, createEsmAndCjsTests, describeWithDockerCompose } from '../../../utils/runner'; @@ -93,6 +94,18 @@ describeWithDockerCompose( 'db.query.text': 'MGET ? ? ?', }), }), + // a failing command on a cache key reports as an errored cache span: + // the span starts as a cache span, so the classification survives the error + expect.objectContaining({ + description: 'dc-cache:list-key', + op: 'cache.get', + status: 'internal_error', + origin: 'auto.db.redis.diagnostic_channel', + data: expect.objectContaining({ + 'cache.operation': 'get', + 'cache.key': ['dc-cache:list-key'], + }), + }), ]), }; @@ -110,6 +123,56 @@ describeWithDockerCompose( .start() .completed(); }); + + test('names cache spans after the cache op with span streaming enabled', { timeout: 60_000 }, async () => { + // With span streaming, cache spans are named after the low-cardinality op + // (`cache.{{cache.operation}}`) instead of the cache key. + const expectedStreamedCacheSpan = (op: string, keys: string[]) => + expect.objectContaining({ + name: op, + is_segment: false, + attributes: expect.objectContaining({ + 'sentry.op': { type: 'string', value: op }, + 'sentry.origin': { type: 'string', value: 'auto.db.redis.diagnostic_channel' }, + 'cache.key': { type: 'array', value: keys }, + }), + }); + + await createTestRunner() + .withEnv({ STREAMED: 'true' }) + // The connect span may be streamed in its own envelope, ahead of the command spans. + .unordered() + .expect({ + span: { + items: expect.arrayContaining([ + expectedStreamedCacheSpan('cache.put', ['dc-cache:test-key']), + expectedStreamedCacheSpan('cache.get', ['dc-cache:test-key']), + expectedStreamedCacheSpan('cache.get', ['dc-cache:unavailable-data']), + // non-cache spans keep their db.query statement name + expect.objectContaining({ name: 'redis-GET' }), + ]), + }, + }) + .start() + .completed(); + }); + + // `ignoreSpans` is evaluated at span start under streaming, so this only passes because the + // span starts as a cache span — a db span renamed at response time would slip through. + test('drops cache spans matching an ignoreSpans op filter at span start', { timeout: 60_000 }, async () => { + await createTestRunner() + .withEnv({ STREAMED: 'true', IGNORE_CACHE_GET: 'true' }) + .unordered() + .expect({ + span: (container: SerializedStreamedSpanContainer) => { + const names = container.items.map(item => item.name); + expect(names).toContain('cache.put'); + expect(names).not.toContain('cache.get'); + }, + }) + .start() + .completed(); + }); }); }, ); diff --git a/packages/server-utils/src/integrations/redis/index.ts b/packages/server-utils/src/integrations/redis/index.ts index f7b5678318a5..12895439e507 100644 --- a/packages/server-utils/src/integrations/redis/index.ts +++ b/packages/server-utils/src/integrations/redis/index.ts @@ -24,7 +24,7 @@ import { import { CHANNELS } from '../../orchestrion/channels'; import { defaultDbStatementSerializer } from './redis-statement-serializer'; import type { RedisCacheOptions } from './redis-cache'; -import { applyRedisCacheAttributes } from './redis-cache'; +import { applyCacheResponseAttributes, getRedisCacheAttributes } from './redis-cache'; import { bindTracingChannelToSpan } from '../../tracing-channel'; import { redisModuleNames } from '../../orchestrion/config/redis'; import { ioredisModuleNames } from '../../orchestrion/config/ioredis'; @@ -99,16 +99,23 @@ function nodeRedisAttributes(options: NodeRedisClientOptions | undefined): SpanA }; } -function startCommandSpan(commandName: string, commandArgs: Array, attributes: SpanAttributes): Span { +function startCommandSpan( + commandName: string, + commandArgs: Array, + attributes: SpanAttributes, + cacheOptions: RedisCacheOptions, +): Span { const dbStatement = defaultDbStatementSerializer(commandName, commandArgs); + const cacheProperties = getRedisCacheAttributes(commandName, commandArgs, attributes, cacheOptions); return startInactiveSpan({ - name: dbStatement || `redis-${commandName}`, + name: cacheProperties?.name ?? (dbStatement || `redis-${commandName}`), attributes: { [SENTRY_KIND]: 'client', ...attributes, [SENTRY_OP]: DB_QUERY, [DB_OPERATION_NAME]: commandName, [DB_QUERY_TEXT]: dbStatement, + ...cacheProperties?.attributes, }, }); } @@ -147,13 +154,13 @@ function subscribeLegacyRedisCommand(cacheOptions: RedisCacheOptions): void { if (client?.connection_options?.port != null) { attributes[SERVER_PORT] = client.connection_options.port; } - const span = startCommandSpan(command.command, command.args ?? [], attributes); + const span = startCommandSpan(command.command, command.args ?? [], attributes, cacheOptions); (data as CommandContext & { _sentrySpan?: Span })._sentrySpan = span; const parentSpan = getActiveSpan(); command.callback = function (this: unknown, err: Error | null | undefined, reply: unknown) { if (!err) { - applyRedisCacheAttributes(span, command.command, command.args ?? [], reply, cacheOptions); + applyCacheResponseAttributes(span, reply); } endSpan(span, err); // eslint-disable-next-line prefer-rest-params @@ -188,16 +195,12 @@ function bindNodeRedisCommandChannel( } const commandName = String(wireArgs[0]); const options = (data.self as NodeRedisClient | undefined)?.options; - return startCommandSpan(commandName, wireArgs.slice(1), nodeRedisAttributes(options)); + return startCommandSpan(commandName, wireArgs.slice(1), nodeRedisAttributes(options), cacheOptions); }, { beforeSpanEnd(span, data) { - if ('error' in data) { - return; - } - const wireArgs = getWireArgs(data); - if (wireArgs?.length) { - applyRedisCacheAttributes(span, String(wireArgs[0]), wireArgs.slice(1), data.result, cacheOptions); + if (!('error' in data)) { + applyCacheResponseAttributes(span, data.result); } }, }, diff --git a/packages/server-utils/src/integrations/redis/ioredis-channel-subscriber.ts b/packages/server-utils/src/integrations/redis/ioredis-channel-subscriber.ts index 2e25ad7c5370..da881e4c43c3 100644 --- a/packages/server-utils/src/integrations/redis/ioredis-channel-subscriber.ts +++ b/packages/server-utils/src/integrations/redis/ioredis-channel-subscriber.ts @@ -9,12 +9,12 @@ import { SERVER_PORT, } from '@sentry/conventions/attributes'; import { DB_QUERY, DB } from '@sentry/conventions/op'; -import type { Span } from '@sentry/core'; +import type { Span, SpanAttributes } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; import { CHANNELS } from '../../orchestrion/channels'; import { bindTracingChannelToSpan } from '../../tracing-channel'; import type { RedisCacheOptions } from './redis-cache'; -import { applyRedisCacheAttributes } from './redis-cache'; +import { applyCacheResponseAttributes, getRedisCacheAttributes } from './redis-cache'; import { defaultDbStatementSerializer } from './redis-statement-serializer'; const ORIGIN = 'auto.db.redis'; @@ -64,7 +64,10 @@ const tracedCommands = new WeakSet(); * * Exported for unit testing. */ -export function startIORedisCommandSpan(data: IORedisCommandContext): Span | undefined { +export function startIORedisCommandSpan( + data: IORedisCommandContext, + cacheOptions: RedisCacheOptions, +): Span | undefined { const command = data.arguments?.[0] as RedisCommand | undefined; if (!command || typeof command !== 'object') { return undefined; @@ -76,15 +79,17 @@ export function startIORedisCommandSpan(data: IORedisCommandContext): Span | und tracedCommands.add(command); const { host, port } = getConnectionOptions(data.self); const statement = defaultDbStatementSerializer(command.name, command.args ?? []); + const attributes: SpanAttributes = { + [SENTRY_KIND]: 'client', + ...connectionAttributes(host, port), + [SENTRY_OP]: DB_QUERY, + [DB_OPERATION_NAME]: command.name, + [DB_QUERY_TEXT]: statement, + }; + const cacheProperties = getRedisCacheAttributes(command.name, command.args ?? [], attributes, cacheOptions); return startInactiveSpan({ - name: statement, - attributes: { - [SENTRY_KIND]: 'client', - ...connectionAttributes(host, port), - [SENTRY_OP]: DB_QUERY, - [DB_OPERATION_NAME]: command.name, - [DB_QUERY_TEXT]: statement, - }, + name: cacheProperties?.name ?? statement, + attributes: { ...attributes, ...cacheProperties?.attributes }, }); } @@ -102,16 +107,12 @@ export function instrumentIoredis(options: RedisCacheOptions): void { CHANNELS.IOREDIS_CONNECT, ); - bindTracingChannelToSpan(commandChannel, startIORedisCommandSpan, { + bindTracingChannelToSpan(commandChannel, data => startIORedisCommandSpan(data, options), { // ioredis' `requireParentSpan` default: only create a span under an active span. requiresParentSpan: true, beforeSpanEnd(span, data) { - if ('error' in data) { - return; - } - const command = data.arguments?.[0] as RedisCommand | undefined; - if (command) { - applyRedisCacheAttributes(span, command.name, command.args, data.result, options); + if (!('error' in data)) { + applyCacheResponseAttributes(span, data.result); } }, }); diff --git a/packages/server-utils/src/integrations/redis/redis-cache.ts b/packages/server-utils/src/integrations/redis/redis-cache.ts index 633149b9fed7..428facdb01c6 100644 --- a/packages/server-utils/src/integrations/redis/redis-cache.ts +++ b/packages/server-utils/src/integrations/redis/redis-cache.ts @@ -1,14 +1,12 @@ import { CACHE_OPERATION, - NET_PEER_NAME, - NET_PEER_PORT, NETWORK_PEER_ADDRESS, NETWORK_PEER_PORT, SERVER_ADDRESS, SERVER_PORT, } from '@sentry/conventions/attributes'; import { CACHE_GET, CACHE_PUT, CACHE_REMOVE } from '@sentry/conventions/op'; -import type { Span } from '@sentry/core'; +import type { Span, SpanAttributes } from '@sentry/core'; import { CACHE_OPERATION_NAMES, getClient, @@ -48,6 +46,10 @@ export interface RedisCacheOptions { * Passing `0` will use the full cache key without truncation. * * By default, the full cache key is used. + * + * Only applies with `traceLifecycle: 'static'`. With span streaming (the default), span names are + * low cardinality: cache spans are named after the cache operation (e.g. `cache.get`) and the + * key is only added to the `cache.key` attribute, so there is nothing to truncate. */ maxCacheKeyLength?: number; } @@ -142,21 +144,22 @@ export function calculateCacheItemSize(response: unknown): number | undefined { } /** - * Turns a redis command span into a cache span when its key matches one of the configured - * `cachePrefixes`: sets the cache op, operation, key, hit/miss and item-size attributes and renames - * the span to the cache key (or, with span streaming, to the low-cardinality cache operation). - * A no-op when no `cachePrefixes` are set or the command/key is not cache-relevant. + * Decides at span-start time whether a redis command is a cache operation (its key matches one of + * the configured `cachePrefixes`) and returns the span name plus attribute overrides to merge into + * the db span options, or `undefined` for a plain db span. Callers must spread the returned + * attributes after their db attributes, so the cache op overrides the db op. Deciding at start time + * — instead of renaming the db span at response time — makes `ignoreSpans` and span streaming see + * the same op/name the user sees in the UI. * - * Runs at command response time against the already-started db span, so it can read connection - * attributes off the span and derive the item size from the response. + * `dbAttributes` are the attributes the caller starts the span with; the network peer is derived + * from `server.address`/`server.port` in there. */ -export function applyRedisCacheAttributes( - span: Span, +export function getRedisCacheAttributes( redisCommand: string, cmdArgs: RedisCommandArgs, - response: unknown, + dbAttributes: SpanAttributes, options: RedisCacheOptions, -): void { +): { name: string; attributes: SpanAttributes } | undefined { const safeKey = getCacheKeySafely(redisCommand, cmdArgs); const cacheOperation = getCacheOperation(redisCommand); @@ -167,52 +170,58 @@ export function applyRedisCacheAttributes( !shouldConsiderForCache(redisCommand, safeKey, options.cachePrefixes) ) { // not relevant for cache - return; - } - - // otel/ioredis seems to be using the old standard, as there was a change to those params: https://github.com/open-telemetry/opentelemetry-specification/issues/3199 - // We are using params based on the docs: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/ - // Fall back to stable semconv attributes (server.address/server.port) when - // old-semconv ones are absent, eg OTEL_SEMCONV_STABILITY_OPT_IN=database - // set for node-redis v4/v5. - const attributes = spanToJSON(span).attributes; - // oxlint-disable-next-line typescript/no-deprecated - const networkPeerAddress = (attributes[NET_PEER_NAME] ?? attributes[SERVER_ADDRESS]) as string | undefined; - // oxlint-disable-next-line typescript/no-deprecated - const networkPeerPort = (attributes[NET_PEER_PORT] ?? attributes[SERVER_PORT]) as number | undefined; - - if (networkPeerPort && networkPeerAddress) { - span.setAttributes({ [NETWORK_PEER_ADDRESS]: networkPeerAddress, [NETWORK_PEER_PORT]: networkPeerPort }); - } - - // A remove response is a delete-count, not a cached value, so its size is meaningless. - const cacheItemSize = isInCommands(REMOVE_COMMANDS, redisCommand) ? undefined : calculateCacheItemSize(response); - - if (cacheItemSize) { - span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); - } - - if (isInCommands(GET_COMMANDS, redisCommand) && cacheItemSize !== undefined) { - span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + return undefined; } - span.setAttributes({ + const attributes: SpanAttributes = { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: cacheOperation, [SEMANTIC_ATTRIBUTE_CACHE_KEY]: safeKey, [CACHE_OPERATION]: CACHE_OPERATION_NAMES[cacheOperation], - }); + }; + + const networkPeerAddress = dbAttributes[SERVER_ADDRESS] as string | undefined; + const networkPeerPort = dbAttributes[SERVER_PORT] as number | undefined; + if (networkPeerPort && networkPeerAddress) { + attributes[NETWORK_PEER_ADDRESS] = networkPeerAddress; + attributes[NETWORK_PEER_PORT] = networkPeerPort; + } const client = getClient(); if (client && hasSpanStreamingEnabled(client)) { // With span streaming, span names have to be low cardinality, so we can't fall back to the cache key. - span.updateName(cacheOperation); - return; + return { name: cacheOperation, attributes }; } // todo: change to string[] once EAP supports it const spanDescription = safeKey.join(', '); - span.updateName(options.maxCacheKeyLength ? truncate(spanDescription, options.maxCacheKeyLength) : spanDescription); + return { + name: options.maxCacheKeyLength ? truncate(spanDescription, options.maxCacheKeyLength) : spanDescription, + attributes, + }; +} + +/** + * Sets the response-derived cache attributes (`cache.hit`, `cache.item_size`) on a span that was + * started as a cache span via {@link getRedisCacheAttributes}. A no-op for plain db spans and for + * `cache.remove` spans — a remove response is a delete-count, not a cached value, so its size is + * meaningless. + */ +export function applyCacheResponseAttributes(span: Span, response: unknown): void { + const op = spanToJSON(span).attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]; + if (op !== CACHE_GET && op !== CACHE_PUT) { + return; + } + + const cacheItemSize = calculateCacheItemSize(response); + + if (cacheItemSize) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize); + } + + if (op === CACHE_GET && cacheItemSize !== undefined) { + span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_HIT, cacheItemSize > 0); + } } type NestedArray = Array | T>; diff --git a/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts b/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts index 830b95aa5e81..9ec0f7d6c83e 100644 --- a/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts +++ b/packages/server-utils/src/integrations/redis/redis-dc-subscriber.ts @@ -9,10 +9,11 @@ import { SENTRY_OP, } from '@sentry/conventions/attributes'; import { DB_QUERY, DB } from '@sentry/conventions/op'; +import type { SpanAttributes } from '@sentry/core'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan } from '@sentry/core'; import { bindTracingChannelToSpan } from '../../tracing-channel'; import type { RedisCacheOptions } from './redis-cache'; -import { applyRedisCacheAttributes } from './redis-cache'; +import { applyCacheResponseAttributes, getRedisCacheAttributes } from './redis-cache'; // Channel names published by node-redis >= 5.12.0 and ioredis >= 5.11.0. // Hardcoded so the subscriber does not have to import either library — the @@ -141,23 +142,25 @@ function setupCommandChannel( // spaces to mirror the format the libraries themselves intend. const args = getCommandArgs(data); const statement = args.length ? `${data.command} ${args.join(' ')}` : data.command; + const attributes: SpanAttributes = { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, + [SENTRY_OP]: DB_QUERY, + [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, + [DB_OPERATION_NAME]: data.command, + [DB_QUERY_TEXT]: statement, + ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), + ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), + }; + const cacheProperties = getRedisCacheAttributes(data.command, args, attributes, cacheOptions); return startInactiveSpan({ - name: `redis-${data.command}`, - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, - [SENTRY_OP]: DB_QUERY, - [DB_SYSTEM_NAME]: DB_SYSTEM_NAME_VALUE_REDIS, - [DB_OPERATION_NAME]: data.command, - [DB_QUERY_TEXT]: statement, - ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), - ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), - }, + name: cacheProperties?.name ?? `redis-${data.command}`, + attributes: { ...attributes, ...cacheProperties?.attributes }, }); }, { beforeSpanEnd(span, data) { if ('error' in data) return; - applyRedisCacheAttributes(span, data.command, getCommandArgs(data), data.result, cacheOptions); + applyCacheResponseAttributes(span, data.result); }, }, ); diff --git a/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts b/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts index b0e21487e95f..8a487330feb5 100644 --- a/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts +++ b/packages/server-utils/test/integrations/redis/ioredis-channel-subscriber.test.ts @@ -21,7 +21,7 @@ describe('startIORedisCommandSpan', () => { }); it('builds a db query span with Sentry convention attributes', () => { - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -39,23 +39,42 @@ describe('startIORedisCommandSpan', () => { ); }); + it('starts the span as a cache span when the key matches a cache prefix', () => { + startIORedisCommandSpan(ctx({ name: 'get', args: ['ioredis-cache:test-key'] }), { + cachePrefixes: ['ioredis-cache:'], + }); + + expect(startInactiveSpanSpy).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'ioredis-cache:test-key', + attributes: expect.objectContaining({ + 'sentry.op': 'cache.get', + 'cache.operation': 'get', + 'cache.key': ['ioredis-cache:test-key'], + 'network.peer.address': 'localhost', + 'network.peer.port': 6379, + }), + }), + ); + }); + it('emits a single span when the same command is re-sent from the offline queue', () => { const command = { name: 'set', args: ['test-key', 'test-value'] }; - expect(startIORedisCommandSpan(ctx(command))).toBeDefined(); - expect(startIORedisCommandSpan(ctx(command))).toBeUndefined(); + expect(startIORedisCommandSpan(ctx(command), {})).toBeDefined(); + expect(startIORedisCommandSpan(ctx(command), {})).toBeUndefined(); expect(startInactiveSpanSpy).toHaveBeenCalledTimes(1); }); it('spans distinct command objects with the same statement', () => { - startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); - startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] })); + startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] }), {}); + startIORedisCommandSpan(ctx({ name: 'get', args: ['k'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledTimes(2); }); it('skips payloads without a command object', () => { - expect(startIORedisCommandSpan({ arguments: [], self: { options: CONNECTION } })).toBeUndefined(); + expect(startIORedisCommandSpan({ arguments: [], self: { options: CONNECTION } }, {})).toBeUndefined(); expect(startInactiveSpanSpy).not.toHaveBeenCalled(); }); }); diff --git a/packages/server-utils/test/integrations/redis/redis-cache.test.ts b/packages/server-utils/test/integrations/redis/redis-cache.test.ts index 326cb813ccc4..aebbc57eb733 100644 --- a/packages/server-utils/test/integrations/redis/redis-cache.test.ts +++ b/packages/server-utils/test/integrations/redis/redis-cache.test.ts @@ -1,8 +1,17 @@ -import { CACHE_KEY, CACHE_OPERATION, SENTRY_OP, SENTRY_SEGMENT_NAME_SOURCE } from '@sentry/conventions/attributes'; -import { setCurrentClient } from '@sentry/core'; -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { - applyRedisCacheAttributes, + CACHE_KEY, + CACHE_OPERATION, + NETWORK_PEER_ADDRESS, + NETWORK_PEER_PORT, + SENTRY_OP, + SERVER_ADDRESS, + SERVER_PORT, +} from '@sentry/conventions/attributes'; +import { SentrySpan, setCurrentClient, spanToJSON } from '@sentry/core'; +import { afterEach, describe, expect, it } from 'vitest'; +import { + applyCacheResponseAttributes, + getRedisCacheAttributes, calculateCacheItemSize, GET_COMMANDS, getCacheKeySafely, @@ -19,75 +28,87 @@ function setUpClient(traceLifecycle: 'stream' | 'static'): void { } describe('redis cache', () => { - describe('applyRedisCacheAttributes', () => { - let mockSpan: any; - - beforeEach(() => { - mockSpan = { - setAttribute: vi.fn(), - setAttributes: vi.fn(), - updateName: vi.fn(), - spanContext: () => ({ spanId: 'test-span-id', traceId: 'test-trace-id' }), - }; + describe('getRedisCacheAttributes', () => { + it.each([ + { desc: 'no args', cmd: 'get', args: [], options: {} }, + { desc: 'unsupported command', cmd: 'exists', args: ['key'], options: {} }, + { desc: 'no cache prefixes', cmd: 'get', args: ['key'], options: {} }, + { desc: 'non-matching prefix', cmd: 'get', args: ['key'], options: { cachePrefixes: ['c'] } }, + ])('should return undefined when $desc', ({ cmd, args, options }) => { + expect(getRedisCacheAttributes(cmd, args, {}, options)).toBeUndefined(); }); - afterEach(() => { - vi.restoreAllMocks(); + it('should return cache op, key and network peer attributes for a matching key', () => { + const result = getRedisCacheAttributes( + 'get', + ['cache:test-key'], + { [SERVER_ADDRESS]: 'localhost', [SERVER_PORT]: 6379 }, + { cachePrefixes: ['cache:'] }, + ); + + expect(result).toStrictEqual({ + name: 'cache:test-key', + attributes: { + [SENTRY_OP]: 'cache.get', + [CACHE_KEY]: ['cache:test-key'], + [CACHE_OPERATION]: 'get', + [NETWORK_PEER_ADDRESS]: 'localhost', + [NETWORK_PEER_PORT]: 6379, + }, + }); }); - describe('early returns', () => { - it.each([ - { desc: 'no args', cmd: 'get', args: [], response: 'test', options: {} }, - { desc: 'unsupported command', cmd: 'exists', args: ['key'], response: 'test', options: {} }, - { desc: 'no cache prefixes', cmd: 'get', args: ['key'], response: 'test', options: {} }, - { desc: 'non-matching prefix', cmd: 'get', args: ['key'], response: 'test', options: { cachePrefixes: ['c'] } }, - ])('should return early without modifying span when $desc', ({ cmd, args, response, options }) => { - applyRedisCacheAttributes(mockSpan, cmd, args, response, options); - - expect(mockSpan.setAttribute).not.toHaveBeenCalled(); - expect(mockSpan.setAttributes).not.toHaveBeenCalled(); - expect(mockSpan.updateName).not.toHaveBeenCalled(); + it('should omit network peer attributes when the db attributes have no server address', () => { + const result = getRedisCacheAttributes('del', ['cache:test-key'], {}, { cachePrefixes: ['cache:'] }); + + expect(result).toStrictEqual({ + name: 'cache:test-key', + attributes: { + [SENTRY_OP]: 'cache.remove', + [CACHE_KEY]: ['cache:test-key'], + [CACHE_OPERATION]: 'remove', + }, }); }); describe('span name truncation', () => { it('should not truncate span name when maxCacheKeyLength is not set', () => { - applyRedisCacheAttributes( - mockSpan, + const result = getRedisCacheAttributes( 'mget', ['cache:very-long-key-name', 'cache:very-long-key-name-2', 'cache:very-long-key-name-3'], - 'value', + {}, { cachePrefixes: ['cache:'] }, ); - expect(mockSpan.updateName).toHaveBeenCalledWith( - 'cache:very-long-key-name, cache:very-long-key-name-2, cache:very-long-key-name-3', - ); - expect(mockSpan.setAttribute).not.toHaveBeenCalledWith(SENTRY_SEGMENT_NAME_SOURCE, undefined); + expect(result?.name).toBe('cache:very-long-key-name, cache:very-long-key-name-2, cache:very-long-key-name-3'); }); it('should truncate span name when maxCacheKeyLength is set', () => { - applyRedisCacheAttributes(mockSpan, 'get', ['cache:very-long-key-name'], 'value', { - cachePrefixes: ['cache:'], - maxCacheKeyLength: 10, - }); + const result = getRedisCacheAttributes( + 'get', + ['cache:very-long-key-name'], + {}, + { + cachePrefixes: ['cache:'], + maxCacheKeyLength: 10, + }, + ); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:very...'); + expect(result?.name).toBe('cache:very...'); }); it('should truncate multiple keys joined with commas', () => { - applyRedisCacheAttributes( - mockSpan, + const result = getRedisCacheAttributes( 'mget', ['cache:key1', 'cache:key2', 'cache:key3'], - ['val1', 'val2', 'val3'], + {}, { cachePrefixes: ['cache:'], maxCacheKeyLength: 20, }, ); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:key1, cache:ke...'); + expect(result?.name).toBe('cache:key1, cache:ke...'); }); }); @@ -103,30 +124,66 @@ describe('redis cache', () => { ])('names a streamed $op span after the cache operation', ({ cmd, op, operation }) => { setUpClient('stream'); - applyRedisCacheAttributes(mockSpan, cmd, ['cache:user-42'], 'value', { cachePrefixes: ['cache:'] }); + const result = getRedisCacheAttributes(cmd, ['cache:user-42'], {}, { cachePrefixes: ['cache:'] }); - expect(mockSpan.updateName).toHaveBeenCalledWith(op); // The key is high cardinality, so it only lives on the attribute. - expect(mockSpan.setAttributes).toHaveBeenCalledWith( - expect.objectContaining({ + expect(result).toStrictEqual({ + name: op, + attributes: { [SENTRY_OP]: op, [CACHE_OPERATION]: operation, [CACHE_KEY]: ['cache:user-42'], - }), - ); + }, + }); }); it('keeps the cache key as the span name when span streaming is off', () => { setUpClient('static'); - applyRedisCacheAttributes(mockSpan, 'get', ['cache:user-42'], 'value', { cachePrefixes: ['cache:'] }); + const result = getRedisCacheAttributes('get', ['cache:user-42'], {}, { cachePrefixes: ['cache:'] }); - expect(mockSpan.updateName).toHaveBeenCalledWith('cache:user-42'); - expect(mockSpan.setAttributes).toHaveBeenCalledWith(expect.objectContaining({ [CACHE_OPERATION]: 'get' })); + expect(result?.name).toBe('cache:user-42'); + expect(result?.attributes).toEqual(expect.objectContaining({ [CACHE_OPERATION]: 'get' })); }); }); }); + describe('applyCacheResponseAttributes', () => { + const cacheSpan = (op: string): SentrySpan => + new SentrySpan({ name: 'cache:test-key', attributes: { [SENTRY_OP]: op } }); + + it('should set item size and cache hit on a cache.get span', () => { + const span = cacheSpan('cache.get'); + applyCacheResponseAttributes(span, 'test-value'); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.item_size': 10, 'cache.hit': true }); + }); + + it('should set a cache miss for an empty cache.get response', () => { + const span = cacheSpan('cache.get'); + applyCacheResponseAttributes(span, null); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.hit': false }); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.item_size'); + }); + + it('should set only the item size on a cache.put span', () => { + const span = cacheSpan('cache.put'); + applyCacheResponseAttributes(span, 'OK'); + + expect(spanToJSON(span).attributes).toMatchObject({ 'cache.item_size': 2 }); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.hit'); + }); + + it.each(['cache.remove', 'db.query'])('should not modify a %s span', op => { + const span = cacheSpan(op); + applyCacheResponseAttributes(span, 'test-value'); + + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.item_size'); + expect(spanToJSON(span).attributes).not.toHaveProperty('cache.hit'); + }); + }); + describe('getCacheKeySafely (single arg)', () => { it('should return an empty string if there are no command arguments', () => { const result = getCacheKeySafely('get', []); From 762802222cfb739e59f1c8a0c00ac0d4d24ddac5 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 2 Sep 2026 17:16:06 +0200 Subject: [PATCH 2/2] assert on client report --- .../suites/tracing/redis-dc/instrument.mjs | 2 ++ .../suites/tracing/redis-dc/test.ts | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs index 2f4c45c014ea..d4f8cc7801fc 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/instrument.mjs @@ -9,4 +9,6 @@ Sentry.init({ transport: loggingTransport, integrations: [Sentry.redisIntegration({ cachePrefixes: ['dc-cache:'] })], ignoreSpans: process.env.IGNORE_CACHE_GET === 'true' ? [{ op: 'cache.get' }] : undefined, + // so the `ignored` span outcomes flush while the scenario is still running + clientReportFlushInterval: 1_000, }); diff --git a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts index 6fcdbcb6988b..db965e173046 100644 --- a/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/redis-dc/test.ts @@ -130,6 +130,9 @@ describeWithDockerCompose( test('drops cache spans matching an ignoreSpans op filter at span start', { timeout: 60_000 }, async () => { await createTestRunner() .withEnv({ STREAMED: 'true', IGNORE_CACHE_GET: 'true' }) + .unignore('client_report') + // The span container and the client report flush on independent timers, so they can + // arrive in either order. .unordered() .expect({ span: (container: SerializedStreamedSpanContainer) => { @@ -138,6 +141,14 @@ describeWithDockerCompose( expect(names).not.toContain('cache.get'); }, }) + .expect({ + client_report: { + discarded_events: [ + // the two GETs on cache keys plus the failing GET, which is also decided at start + { category: 'span', quantity: 3, reason: 'ignored' }, + ], + }, + }) .start() .completed(); });