Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(redis-cache): Create cache-span with prefixed keys (get/set commands) #12070

Merged
merged 7 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: '3.9'

services:
db:
image: redis:latest
restart: always
container_name: integration-tests-redis
ports:
- '6379:6379'
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
const Sentry = require('@sentry/node');

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
tracesSampleRate: 1.0,
transport: loggingTransport,
integrations: [Sentry.redisIntegration({ cachePrefixes: ['ioredis-cache:'] })],
});

// Stop the process from exiting before the transaction is sent
setInterval(() => {}, 1000);

const Redis = require('ioredis');

const redis = new Redis({ port: 6379 });

async function run() {
await Sentry.startSpan(
{
name: 'Test Transaction',
op: 'transaction',
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
name: 'Test Transaction',
op: 'transaction',
},
name: 'Test Span',
op: 'test-span',
},

let's avoid transaction wording in new code :D

async () => {
try {
await redis.set('test-key', 'test-value');
await redis.set('ioredis-cache:test-key', 'test-value');

await redis.get('test-key');
await redis.get('ioredis-cache:test-key');
await redis.get('ioredis-cache:unavailable-data');
} finally {
await redis.disconnect();
}
},
);
}

// eslint-disable-next-line @typescript-eslint/no-floating-promises
run();
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

describe('redis auto instrumentation', () => {
afterAll(() => {
cleanupChildProcesses();
});

test('should not add cache spans when key is not prefixed', done => {
const EXPECTED_TRANSACTION = {
transaction: 'Test Transaction',
spans: expect.arrayContaining([
expect.objectContaining({
description: 'set test-key [1 other arguments]',
op: 'db',
data: expect.objectContaining({
'sentry.op': 'db',
'db.system': 'redis',
'net.peer.name': 'localhost',
'net.peer.port': 6379,
'db.statement': 'set test-key [1 other arguments]',
}),
}),
expect.objectContaining({
description: 'get test-key',
op: 'db',
data: expect.objectContaining({
'sentry.op': 'db',
'db.system': 'redis',
'net.peer.name': 'localhost',
'net.peer.port': 6379,
'db.statement': 'get test-key',
}),
}),
]),
};

createRunner(__dirname, 'scenario-ioredis.js')
.withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] })
.expect({ transaction: EXPECTED_TRANSACTION })
.start(done);
});

test('should create cache spans for prefixed keys', done => {
const EXPECTED_TRANSACTION = {
transaction: 'Test Transaction',
spans: expect.arrayContaining([
// SET
expect.objectContaining({
description: 'set ioredis-cache:test-key [1 other arguments]',
op: 'cache.put',
data: expect.objectContaining({
'db.statement': 'set ioredis-cache:test-key [1 other arguments]',
'cache.key': 'ioredis-cache:test-key',
'cache.item_size': 2,
'network.peer.address': 'localhost',
'network.peer.port': 6379,
}),
}),
// GET
expect.objectContaining({
description: 'get ioredis-cache:test-key',
op: 'cache.get_item', // todo: will be changed to cache.get
data: expect.objectContaining({
'db.statement': 'get ioredis-cache:test-key',
'cache.hit': true,
'cache.key': 'ioredis-cache:test-key',
'cache.item_size': 10,
'network.peer.address': 'localhost',
'network.peer.port': 6379,
}),
}),
// GET (unavailable)
expect.objectContaining({
description: 'get ioredis-cache:unavailable-data',
op: 'cache.get_item', // todo: will be changed to cache.get
data: expect.objectContaining({
'db.statement': 'get ioredis-cache:unavailable-data',
'cache.hit': false,
'cache.key': 'ioredis-cache:test-key',
'network.peer.address': 'localhost',
'network.peer.port': 6379,
}),
}),
]),
};

createRunner(__dirname, 'scenario-ioredis.js')
.withDockerCompose({ workingDirectory: [__dirname], readyMatches: ['port=6379'] })
.expect({ transaction: EXPECTED_TRANSACTION })
.start(done);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('redis auto instrumentation', () => {
description: 'set test-key [1 other arguments]',
op: 'db',
data: expect.objectContaining({
'sentry.op': 'db',
'db.system': 'redis',
'net.peer.name': 'localhost',
'net.peer.port': 6379,
Expand All @@ -23,6 +24,7 @@ describe('redis auto instrumentation', () => {
description: 'get test-key',
op: 'db',
data: expect.objectContaining({
'sentry.op': 'db',
'db.system': 'redis',
'net.peer.name': 'localhost',
'net.peer.port': 6379,
Expand Down
6 changes: 6 additions & 0 deletions packages/core/src/semanticAttributes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,9 @@ export const SEMANTIC_ATTRIBUTE_SENTRY_MEASUREMENT_VALUE = 'sentry.measurement_v
export const SEMANTIC_ATTRIBUTE_PROFILE_ID = 'sentry.profile_id';

export const SEMANTIC_ATTRIBUTE_EXCLUSIVE_TIME = 'sentry.exclusive_time';

export const SEMANTIC_ATTRIBUTE_CACHE_HIT = 'cache.hit';

export const SEMANTIC_ATTRIBUTE_CACHE_KEY = 'cache.key';

export const SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE = 'cache.item_size';
77 changes: 74 additions & 3 deletions packages/node/src/integrations/tracing/redis.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,85 @@
import { IORedisInstrumentation } from '@opentelemetry/instrumentation-ioredis';
import { defineIntegration } from '@sentry/core';
import {
SEMANTIC_ATTRIBUTE_CACHE_HIT,
SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE,
SEMANTIC_ATTRIBUTE_CACHE_KEY,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
defineIntegration,
spanToJSON,
} from '@sentry/core';
import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
import type { IntegrationFn } from '@sentry/types';

const _redisIntegration = (() => {
function keyHasPrefix(key: string, prefixes: string[]): boolean {
return prefixes.some(prefix => key.startsWith(prefix));
}

/** Currently, caching only supports 'get' and 'set' commands. More commands will be added (setex, mget, del, expire) */
function shouldConsiderForCache(
redisCommand: string,
// eslint-disable-next-line @typescript-eslint/no-explicit-any
key: string | number | any[] | Buffer,
prefixes: string[],
): boolean {
return (redisCommand === 'get' || redisCommand === 'set') && typeof key === 'string' && keyHasPrefix(key, prefixes);
}

function calculateCacheItemSize(response: unknown): number {
try {
if (Buffer.isBuffer(response)) return response.byteLength;
else if (typeof response === 'string') return response.length;
else if (typeof response === 'number') return response.toString().length;
else if (response === null || response === undefined) return 0;
return JSON.stringify(response).length;
} catch (e) {
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m: Should we return undefined here? Maybe it is more confusing to set this to 0 if something fails, than to not set it at all?

}
}

interface RedisOptions {
cachePrefixes?: string[];
}

const _redisIntegration = ((options?: RedisOptions) => {
return {
name: 'Redis',
setupOnce() {
addOpenTelemetryInstrumentation([
new IORedisInstrumentation({}),
new IORedisInstrumentation({
responseHook: (span, redisCommand, cmdArgs, response) => {
const key = cmdArgs[0];

if (!options?.cachePrefixes || !shouldConsiderForCache(redisCommand, key, options.cachePrefixes)) {
// not relevant for cache
return;
}

const networkPeerAddress = spanToJSON(span).data?.['net.peer.name'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of interest, are we using something non-standard here or is redis emitting something non-standard?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redis seems to be using the old standard as there was a change to those params: open-telemetry/opentelemetry-specification#3199

We are using those: https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, got it, makes sense - maybe we can leave a comment like this in the code so we can potentially revisit this when we update the redis instrumentation at some point :)

const networkPeerPort = spanToJSON(span).data?.['net.peer.port'];
span.setAttributes({ 'network.peer.address': networkPeerAddress, 'network.peer.port': networkPeerPort });

const cacheItemSize = calculateCacheItemSize(response);
span.setAttribute(SEMANTIC_ATTRIBUTE_CACHE_ITEM_SIZE, cacheItemSize);

if (typeof key === 'string') {
switch (redisCommand) {
case 'get':
span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.get_item',
[SEMANTIC_ATTRIBUTE_CACHE_KEY]: key,
[SEMANTIC_ATTRIBUTE_CACHE_HIT]: cacheItemSize > 0,
});
break;
case 'set':
span.setAttributes({
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'cache.put',
[SEMANTIC_ATTRIBUTE_CACHE_KEY]: key,
});
break;
}
}
},
}),
// todo: implement them gradually
// new LegacyRedisInstrumentation({}),
// new RedisInstrumentation({}),
Expand Down
Loading