diff --git a/MIGRATION.md b/MIGRATION.md index dadca8ed4cc2..567e076eeb9a 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -999,6 +999,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. Redis has no SQL statement to summarize and no collection to pair a command with, so redis and ioredis `db.query` spans are named after the operation and the connection instead of the command that was sent. The command and its arguments remain available on `db.query.text`, redacted as before. `MULTI`/`PIPELINE` batch spans are unchanged — they were already named after their operation, which they now also report on `db.operation.name`. `db.namespace` is deliberately not used in the name: for redis it is the numeric database index, which says nothing about what the command did. 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 c0a1998369a5..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 @@ -8,4 +8,7 @@ Sentry.init({ tracesSampleRate: 1.0, 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/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 a02326b0a1cf..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 @@ -24,7 +24,7 @@ describeWithDockerCompose( 'db.query.text': 'SET dc-test-key ?', }), }), - // cache SET: span name updated to key by cacheResponseHook + // cache SET: starts as a cache span, named by its key in the static lifecycle expect.objectContaining({ description: 'dc-cache:test-key', op: 'cache.put', @@ -95,6 +95,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'], + }), + }), ]), }; @@ -112,6 +124,34 @@ describeWithDockerCompose( .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' }) + .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) => { + const names = container.items.map(item => item.name); + expect(names).toContain('cache.put'); + 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(); + }); }); // The same commands as above, asserted on the streamed span container. With span streaming the @@ -165,8 +205,8 @@ describeWithDockerCompose( const PEER = { 'network.peer.address': HOST, 'network.peer.port': PORT }; - // A cache span is a db span the cache hook took over: it is renamed to its cache operation - // and reports the connection it inherited as peer attributes too. + // A cache span is a db span whose key matched a cache prefix: it starts named after its + // cache operation and reports the connection as peer attributes too. const cacheSpan = ( op: 'cache.get' | 'cache.put' | 'cache.remove', attributes: Record, @@ -194,7 +234,7 @@ describeWithDockerCompose( 'db.operation.name': 'SET', 'db.query.text': 'SET dc-test-key ?', }), - // cache SET: turned into a cache span, and renamed by the cache hook + // cache SET: starts as a cache span cacheSpan('cache.put', { 'db.operation.name': 'SET', 'db.query.text': 'SET dc-cache:test-key ?', @@ -233,6 +273,22 @@ describeWithDockerCompose( 'db.operation.name': 'MGET', 'db.query.text': 'MGET ? ? ?', }), + streamedSpan(`LPUSH ${HOST}:${PORT}`, 'db.query', { + 'db.operation.name': 'LPUSH', + 'db.query.text': 'LPUSH dc-cache:list-key ?', + }), + // 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 + { + ...(cacheSpan('cache.get', { + 'db.operation.name': 'GET', + 'db.query.text': 'GET dc-cache:list-key', + 'cache.key': ['dc-cache:list-key'], + 'error.type': 'Error', + 'sentry.status.message': 'WRONGTYPE Operation against a key holding the wrong kind of value', + }) as Record), + status: 'error', + }, ]); }, }) diff --git a/packages/server-utils/src/integrations/redis/index.ts b/packages/server-utils/src/integrations/redis/index.ts index 9241f4936e80..05e4135ab63c 100644 --- a/packages/server-utils/src/integrations/redis/index.ts +++ b/packages/server-utils/src/integrations/redis/index.ts @@ -25,7 +25,7 @@ import { CHANNELS } from '../../orchestrion/channels'; import { getRedisQueryNaming } from './redis-span-name'; 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'; @@ -108,15 +108,21 @@ 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 { streamedName, attributes: namingAttributes } = getRedisQueryNaming(commandName, commandArgs, { host: attributes[SERVER_ADDRESS], port: attributes[SERVER_PORT], }); + const cacheProperties = getRedisCacheAttributes(commandName, commandArgs, attributes, cacheOptions); return startInactiveSpan({ - name: streamedName || dbStatement || `redis-${commandName}`, + name: cacheProperties?.name ?? streamedName ?? (dbStatement || `redis-${commandName}`), attributes: { [SENTRY_KIND]: 'client', ...attributes, @@ -124,6 +130,7 @@ function startCommandSpan(commandName: string, commandArgs: Array(); * * 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; @@ -81,17 +84,19 @@ export function startIORedisCommandSpan(data: IORedisCommandContext): Span | und host, port, }); + const attributes: SpanAttributes = { + [SENTRY_KIND]: 'client', + ...connectionAttributes(host, port), + [SENTRY_OP]: DB_QUERY, + [DB_OPERATION_NAME]: command.name, + ...namingAttributes, + [DB_QUERY_TEXT]: statement, + }; + const cacheProperties = getRedisCacheAttributes(command.name, command.args ?? [], attributes, cacheOptions); return startInactiveSpan({ - name: streamedName || statement, - attributes: { - [SENTRY_KIND]: 'client', - ...connectionAttributes(host, port), - [SENTRY_OP]: DB_QUERY, - [DB_OPERATION_NAME]: command.name, - ...namingAttributes, - [DB_QUERY_TEXT]: statement, - }, + name: cacheProperties?.name ?? streamedName ?? statement, + attributes: { ...attributes, ...cacheProperties?.attributes }, }); } @@ -109,16 +114,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 8ff655078f3c..090b02d7272e 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'; import { getRedisQueryNaming } from './redis-span-name'; // Channel names published by node-redis >= 5.12.0 and ioredis >= 5.11.0. @@ -146,24 +147,26 @@ function setupCommandChannel( host: data.serverAddress, port: data.serverPort, }); + 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, + ...namingAttributes, + [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: streamedName || `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, - ...namingAttributes, - [DB_QUERY_TEXT]: statement, - ...(data.serverAddress != null ? { [SERVER_ADDRESS]: data.serverAddress } : {}), - ...(data.serverPort != null ? { [SERVER_PORT]: data.serverPort } : {}), - }, + name: cacheProperties?.name ?? streamedName ?? `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 4ed2fb440ec4..37cde9449001 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 @@ -24,7 +24,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({ @@ -42,12 +42,31 @@ 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('names the span from the conventions with span streaming enabled', () => { vi.spyOn(SentryCore, 'getClient').mockReturnValue({ getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -66,7 +85,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'fcall', args: ['my_func', '1', 'test-key'] })); + startIORedisCommandSpan(ctx({ name: 'fcall', args: ['my_func', '1', 'test-key'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -85,7 +104,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'fcall', args: ['?', '1', 'test-key'] })); + startIORedisCommandSpan(ctx({ name: 'fcall', args: ['?', '1', 'test-key'] }), {}); expect(startInactiveSpanSpy).toHaveBeenCalledWith( expect.objectContaining({ @@ -100,7 +119,7 @@ describe('startIORedisCommandSpan', () => { getOptions: () => ({ traceLifecycle: 'stream' }), } as unknown as ReturnType); - startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }, { port: 6379 })); + startIORedisCommandSpan(ctx({ name: 'set', args: ['test-key', 'test-value'] }, { port: 6379 }), {}); // `{db.system.name}` — the address/port template needs both halves expect(startInactiveSpanSpy).toHaveBeenCalledWith(expect.objectContaining({ name: 'redis' })); @@ -109,20 +128,20 @@ describe('startIORedisCommandSpan', () => { 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', []);