Skip to content

Commit 99ab12b

Browse files
Lms24claude
andcommitted
feat(server-utils): Report node-redis' own connection defaults
node-redis v4 writes its `localhost:6379` socket defaults back into `client.options`, v5 does not. Reading the options as-is therefore reported `server.address`/`server.port` for a v4 client and neither for the identically configured v5 one — so the same client got `SET localhost:6383` on v4 and the bare `redis` fallback on v5 once span names became low cardinality. Resolve the connection the way node-redis >= 5.12 reports it on its own diagnostics channel instead: a TCP client falls back to `localhost:6379`, and a unix socket reports its path as the address and no port. Batch spans go through the same helper rather than repeating the option lookup. The redis-cache suite picks up the low-cardinality cache names from #23830 in the same pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f71e8c4 commit 99ab12b

2 files changed

Lines changed: 65 additions & 52 deletions

File tree

  • dev-packages/node-integration-tests/suites/tracing/redis-cache
  • packages/server-utils/src/integrations/redis

dev-packages/node-integration-tests/suites/tracing/redis-cache/test.ts

Lines changed: 52 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -684,8 +684,8 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
684684
// The blocks above assert the same commands as transactions. With span streaming, span names
685685
// have to be low cardinality, so `db.query` spans drop the serialized statement from their
686686
// name — it stays on `db.query.text` — and are named
687-
// `{db.operation.name} {server.address}:{server.port}` instead. Cache spans are still named
688-
// after the cache key by the cache hook, and batch spans keep their `MULTI`/`PIPELINE` name.
687+
// `{db.operation.name} {server.address}:{server.port}` instead. Cache spans are named after
688+
// their cache operation, and batch spans keep their `MULTI`/`PIPELINE` name.
689689
const streamAttribute = (value: unknown): { type: string; value: unknown } => ({
690690
type: Array.isArray(value) ? 'array' : Number.isInteger(value) ? 'integer' : typeof value,
691691
value,
@@ -751,6 +751,13 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
751751
const span = (name: string, op: string, attributes: Record<string, unknown>, status?: string): unknown =>
752752
streamedSpan({ name, op, segmentName, status, attributes: { ...connection, ...attributes } });
753753

754+
// A cache span is a db span the cache hook took over: it is renamed to its cache operation
755+
// and reports the connection it inherited as peer attributes too.
756+
const cacheSpan = (
757+
op: 'cache.get' | 'cache.put' | 'cache.remove',
758+
attributes: Record<string, unknown>,
759+
): unknown => span(op, op, { ...peer, 'cache.operation': op.slice('cache.'.length), ...attributes });
760+
754761
createEsmAndCjsTests(__dirname, 'scenario-ioredis.mjs', 'instrument-ioredis.mjs', (createTestRunner, test) => {
755762
test('creates streamed db and cache spans (ioredis)', { timeout: 60_000 }, async () => {
756763
await createTestRunner()
@@ -764,22 +771,19 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
764771
'db.operation.name': 'set',
765772
'db.query.text': 'set test-key [1 other arguments]',
766773
}),
767-
span('ioredis-cache:test-key', 'cache.put', {
768-
...peer,
774+
cacheSpan('cache.put', {
769775
'db.operation.name': 'set',
770776
'db.query.text': 'set ioredis-cache:test-key [1 other arguments]',
771777
'cache.key': ['ioredis-cache:test-key'],
772778
'cache.item_size': 2,
773779
}),
774-
span('ioredis-cache:test-key-set-EX', 'cache.put', {
775-
...peer,
780+
cacheSpan('cache.put', {
776781
'db.operation.name': 'set',
777782
'db.query.text': 'set ioredis-cache:test-key-set-EX [3 other arguments]',
778783
'cache.key': ['ioredis-cache:test-key-set-EX'],
779784
'cache.item_size': 2,
780785
}),
781-
span('ioredis-cache:test-key-setex', 'cache.put', {
782-
...peer,
786+
cacheSpan('cache.put', {
783787
'db.operation.name': 'setex',
784788
'db.query.text': 'setex ioredis-cache:test-key-setex [2 other arguments]',
785789
'cache.key': ['ioredis-cache:test-key-setex'],
@@ -789,31 +793,27 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
789793
'db.operation.name': 'get',
790794
'db.query.text': 'get test-key',
791795
}),
792-
span('ioredis-cache:test-key', 'cache.get', {
793-
...peer,
796+
cacheSpan('cache.get', {
794797
'db.operation.name': 'get',
795798
'db.query.text': 'get ioredis-cache:test-key',
796799
'cache.key': ['ioredis-cache:test-key'],
797800
'cache.hit': true,
798801
'cache.item_size': 10,
799802
}),
800-
span('ioredis-cache:unavailable-data', 'cache.get', {
801-
...peer,
803+
cacheSpan('cache.get', {
802804
'db.operation.name': 'get',
803805
'db.query.text': 'get ioredis-cache:unavailable-data',
804806
'cache.key': ['ioredis-cache:unavailable-data'],
805807
'cache.hit': false,
806808
}),
807-
span('test-key, ioredis-cache:test-key, ioredis-cache:unavailable-data', 'cache.get', {
808-
...peer,
809+
cacheSpan('cache.get', {
809810
'db.operation.name': 'mget',
810811
'db.query.text': 'mget [3 other arguments]',
811812
'cache.key': ['test-key', 'ioredis-cache:test-key', 'ioredis-cache:unavailable-data'],
812813
'cache.hit': true,
813814
'cache.item_size': 20,
814815
}),
815-
span('ioredis-cache:test-key', 'cache.remove', {
816-
...peer,
816+
cacheSpan('cache.remove', {
817817
'db.operation.name': 'del',
818818
'db.query.text': 'del ioredis-cache:test-key',
819819
'cache.key': ['ioredis-cache:test-key'],
@@ -837,6 +837,13 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
837837
const span = (name: string, op: string, attributes: Record<string, unknown>, status?: string): unknown =>
838838
streamedSpan({ name, op, segmentName, status, attributes: { ...connection, ...attributes } });
839839

840+
// A cache span is a db span the cache hook took over: it is renamed to its cache operation
841+
// and reports the connection it inherited as peer attributes too.
842+
const cacheSpan = (
843+
op: 'cache.get' | 'cache.put' | 'cache.remove',
844+
attributes: Record<string, unknown>,
845+
): unknown => span(op, op, { ...peer, 'cache.operation': op.slice('cache.'.length), ...attributes });
846+
840847
createEsmAndCjsTests(__dirname, 'scenario-redis-4.mjs', 'instrument-redis-4.mjs', (createTestRunner, test) => {
841848
test('creates streamed db and cache spans (redis-4)', { timeout: 60_000 }, async () => {
842849
await createTestRunner()
@@ -855,22 +862,19 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
855862
'db.operation.name': 'SET',
856863
'db.query.text': 'SET redis-test-key [1 other arguments]',
857864
}),
858-
span('redis-cache:test-key', 'cache.put', {
859-
...peer,
865+
cacheSpan('cache.put', {
860866
'db.operation.name': 'SET',
861867
'db.query.text': 'SET redis-cache:test-key [1 other arguments]',
862868
'cache.key': ['redis-cache:test-key'],
863869
'cache.item_size': 2,
864870
}),
865-
span('redis-cache:test-key-set-EX', 'cache.put', {
866-
...peer,
871+
cacheSpan('cache.put', {
867872
'db.operation.name': 'SET',
868873
'db.query.text': 'SET redis-cache:test-key-set-EX [3 other arguments]',
869874
'cache.key': ['redis-cache:test-key-set-EX'],
870875
'cache.item_size': 2,
871876
}),
872-
span('redis-cache:test-key-setex', 'cache.put', {
873-
...peer,
877+
cacheSpan('cache.put', {
874878
'db.operation.name': 'SETEX',
875879
'db.query.text': 'SETEX redis-cache:test-key-setex [2 other arguments]',
876880
'cache.key': ['redis-cache:test-key-setex'],
@@ -880,31 +884,27 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
880884
'db.operation.name': 'GET',
881885
'db.query.text': 'GET redis-test-key',
882886
}),
883-
span('redis-cache:test-key', 'cache.get', {
884-
...peer,
887+
cacheSpan('cache.get', {
885888
'db.operation.name': 'GET',
886889
'db.query.text': 'GET redis-cache:test-key',
887890
'cache.key': ['redis-cache:test-key'],
888891
'cache.hit': true,
889892
'cache.item_size': 10,
890893
}),
891-
span('redis-cache:unavailable-data', 'cache.get', {
892-
...peer,
894+
cacheSpan('cache.get', {
893895
'db.operation.name': 'GET',
894896
'db.query.text': 'GET redis-cache:unavailable-data',
895897
'cache.key': ['redis-cache:unavailable-data'],
896898
'cache.hit': false,
897899
}),
898-
span('redis-test-key, redis-cache:test-key, redis-cache:unavailable-data', 'cache.get', {
899-
...peer,
900+
cacheSpan('cache.get', {
900901
'db.operation.name': 'MGET',
901902
'db.query.text': 'MGET [3 other arguments]',
902903
'cache.key': ['redis-test-key', 'redis-cache:test-key', 'redis-cache:unavailable-data'],
903904
'cache.hit': true,
904905
'cache.item_size': 20,
905906
}),
906-
span('redis-cache:test-key', 'cache.remove', {
907-
...peer,
907+
cacheSpan('cache.remove', {
908908
'db.operation.name': 'DEL',
909909
'db.query.text': 'DEL redis-cache:test-key',
910910
'cache.key': ['redis-cache:test-key'],
@@ -931,16 +931,24 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
931931
});
932932
});
933933

934-
// node-redis v5 leaves `socket.host` unset when only a port is passed, so there is no
935-
// `server.address` to pair the operation with and the span name falls back to
936-
// `{db.system.name}`.
934+
// node-redis v5 leaves `socket.host` unset when only a port is passed, unlike v4. The
935+
// integration fills in the library's own `localhost` default, so both report the same
936+
// connection and get the same span name.
937937
describe('redis-5', () => {
938938
const segmentName = 'Test Span Redis 5';
939-
const connection = { 'server.port': 6383 };
939+
const connection = { 'server.address': 'localhost', 'server.port': 6383 };
940+
const peer = { 'network.peer.address': 'localhost', 'network.peer.port': 6383 };
940941

941942
const span = (name: string, op: string, attributes: Record<string, unknown>, status?: string): unknown =>
942943
streamedSpan({ name, op, segmentName, status, attributes: { ...connection, ...attributes } });
943944

945+
// A cache span is a db span the cache hook took over: it is renamed to its cache operation
946+
// and reports the connection it inherited as peer attributes too.
947+
const cacheSpan = (
948+
op: 'cache.get' | 'cache.put' | 'cache.remove',
949+
attributes: Record<string, unknown>,
950+
): unknown => span(op, op, { ...peer, 'cache.operation': op.slice('cache.'.length), ...attributes });
951+
944952
createEsmAndCjsTests(__dirname, 'scenario-redis-5.mjs', 'instrument-redis-5.mjs', (createTestRunner, test) => {
945953
test('creates streamed db and cache spans (redis-5)', { timeout: 60_000 }, async () => {
946954
await createTestRunner()
@@ -953,60 +961,60 @@ describeWithDockerCompose('redis cache auto instrumentation', { workingDirectory
953961
]);
954962

955963
expect(childSpans(container)).toEqual([
956-
span('redis', redisSpanOp, {
964+
span('SET localhost:6383', redisSpanOp, {
957965
'db.operation.name': 'SET',
958966
'db.query.text': 'SET redis-5-test-key [1 other arguments]',
959967
}),
960-
span('redis-5-cache:test-key', 'cache.put', {
968+
cacheSpan('cache.put', {
961969
'db.operation.name': 'SET',
962970
'db.query.text': 'SET redis-5-cache:test-key [1 other arguments]',
963971
'cache.key': ['redis-5-cache:test-key'],
964972
'cache.item_size': 2,
965973
}),
966-
span('redis-5-cache:test-key-set-EX', 'cache.put', {
974+
cacheSpan('cache.put', {
967975
'db.operation.name': 'SET',
968976
'db.query.text': 'SET redis-5-cache:test-key-set-EX [3 other arguments]',
969977
'cache.key': ['redis-5-cache:test-key-set-EX'],
970978
'cache.item_size': 2,
971979
}),
972-
span('redis-5-cache:test-key-setex', 'cache.put', {
980+
cacheSpan('cache.put', {
973981
'db.operation.name': 'SETEX',
974982
'db.query.text': 'SETEX redis-5-cache:test-key-setex [2 other arguments]',
975983
'cache.key': ['redis-5-cache:test-key-setex'],
976984
'cache.item_size': 2,
977985
}),
978-
span('redis', redisSpanOp, {
986+
span('GET localhost:6383', redisSpanOp, {
979987
'db.operation.name': 'GET',
980988
'db.query.text': 'GET redis-5-test-key',
981989
}),
982-
span('redis-5-cache:test-key', 'cache.get', {
990+
cacheSpan('cache.get', {
983991
'db.operation.name': 'GET',
984992
'db.query.text': 'GET redis-5-cache:test-key',
985993
'cache.key': ['redis-5-cache:test-key'],
986994
'cache.hit': true,
987995
'cache.item_size': 10,
988996
}),
989-
span('redis-5-cache:unavailable-data', 'cache.get', {
997+
cacheSpan('cache.get', {
990998
'db.operation.name': 'GET',
991999
'db.query.text': 'GET redis-5-cache:unavailable-data',
9921000
'cache.key': ['redis-5-cache:unavailable-data'],
9931001
'cache.hit': false,
9941002
}),
995-
span('redis-5-test-key, redis-5-cache:test-key, redis-5-cache:unavailable-data', 'cache.get', {
1003+
cacheSpan('cache.get', {
9961004
'db.operation.name': 'MGET',
9971005
'db.query.text': 'MGET [3 other arguments]',
9981006
'cache.key': ['redis-5-test-key', 'redis-5-cache:test-key', 'redis-5-cache:unavailable-data'],
9991007
'cache.hit': true,
10001008
'cache.item_size': 20,
10011009
}),
1002-
span('redis-5-cache:test-key', 'cache.remove', {
1010+
cacheSpan('cache.remove', {
10031011
'db.operation.name': 'DEL',
10041012
'db.query.text': 'DEL redis-5-cache:test-key',
10051013
'cache.key': ['redis-5-cache:test-key'],
10061014
}),
10071015
span('MULTI', redisSpanOp, { 'db.operation.name': 'MULTI', 'db.operation.batch.size': 2 }),
10081016
span(
1009-
'redis',
1017+
'INCR localhost:6383',
10101018
redisSpanOp,
10111019
{
10121020
'db.operation.name': 'INCR',

packages/server-utils/src/integrations/redis/index.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ interface LegacyRedisClient {
5959
}
6060

6161
interface NodeRedisClientOptions {
62-
socket?: { host?: string; port?: number };
62+
socket?: { host?: string; port?: number; path?: string };
6363
url?: string;
6464
}
6565

@@ -91,11 +91,19 @@ function stripCommandOptions(args: unknown[]): unknown[] {
9191
return args;
9292
}
9393

94+
// Resolves the connection the way node-redis >= 5.12 reports it on its own diagnostics channel: a
95+
// unix socket reports its path and no port, a TCP client the `localhost:6379` it defaults to.
96+
// Only v4 writes those defaults back into `client.options`, so reading the options as-is would
97+
// report a connection for a v4 client and none for the identically configured v5 one.
9498
function nodeRedisAttributes(options: NodeRedisClientOptions | undefined): SpanAttributes {
99+
const socket = options?.socket;
100+
const host = socket?.path ?? socket?.host ?? 'localhost';
101+
const port = socket?.path != null ? undefined : (socket?.port ?? 6379);
102+
95103
return {
96104
[DB_SYSTEM_NAME]: DB_SYSTEM_VALUE_REDIS,
97-
...(options?.socket?.host != null ? { [SERVER_ADDRESS]: options.socket.host } : {}),
98-
...(options?.socket?.port != null ? { [SERVER_PORT]: options.socket.port } : {}),
105+
[SERVER_ADDRESS]: host,
106+
...(port != null ? { [SERVER_PORT]: port } : {}),
99107
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
100108
};
101109
}
@@ -255,19 +263,16 @@ function bindNodeRedisBatchChannel(channelName: string, getOperation: (data: Com
255263
bindTracingChannelToSpan(channel, data => {
256264
const commands = data.arguments?.[0];
257265
const size = Array.isArray(commands) ? commands.length : undefined;
258-
const socket = (data.self as NodeRedisClient | undefined)?.options?.socket;
266+
const options = (data.self as NodeRedisClient | undefined)?.options;
259267
const operation = getOperation(data);
260268
return startInactiveSpan({
261269
name: operation,
262270
attributes: {
263271
[SENTRY_KIND]: 'client',
264-
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
272+
...nodeRedisAttributes(options),
265273
[SENTRY_OP]: DB_QUERY,
266-
[DB_SYSTEM_NAME]: DB_SYSTEM_VALUE_REDIS,
267274
[DB_OPERATION_NAME]: operation,
268275
...(size && size > 1 ? { [DB_OPERATION_BATCH_SIZE]: size } : {}),
269-
...(socket?.host != null ? { [SERVER_ADDRESS]: socket.host } : {}),
270-
...(socket?.port != null ? { [SERVER_PORT]: socket.port } : {}),
271276
},
272277
});
273278
});

0 commit comments

Comments
 (0)