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.

Sign up for GitHub

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(ioredis): only serialize non sensitive arguments in db statement attribute #1052

Merged
merged 6 commits into from
Jun 15, 2022
Merged

feat(ioredis): only serialize non sensitive arguments in db statement attribute #1052

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9f099e6
feat(ioredis): only serialize first argument in redis command
aptomaKetil Jun 8, 2022
be2b576
feat(ioredis): specify serialization subsets per command
aptomaKetil Jun 9, 2022
3601dd0
feat(ioredis): update command list, fallback to serialize one argument
aptomaKetil Jun 10, 2022
d32b872
fix(ioredis): improve readability, add test for serializer
aptomaKetil Jun 12, 2022
5f17938
fix(ioredis): fall back to serializing no arguments
aptomaKetil Jun 14, 2022
a2b8ffe
Merge branch 'main' into ioredis-update-serializer
vmarchaud Jun 15, 2022
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
4 changes: 3 additions & 1 deletion plugins/node/opentelemetry-instrumentation-ioredis/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ IORedis instrumentation has few options available to choose from. You can set th

#### Custom db.statement Serializer

The instrumentation serializes the whole command into a Span attribute called `db.statement`. The standard serialization format is `{cmdName} {cmdArgs.join(',')}`.
The instrumentation serializes the command into a Span attribute called `db.statement`. The standard serialization format attempts to be as informative
as possible while avoiding the export of potentially sensitive data. The number of serialized arguments depends on the specific command, see the configuration
list in `src/utils.ts`.
It is also possible to define a custom serialization function. The function will receive the command name and arguments and must return a string.

Here is a simple example to serialize the command name skipping arguments:
Expand Down
45 changes: 41 additions & 4 deletions plugins/node/opentelemetry-instrumentation-ioredis/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,34 @@
import { Span, SpanStatusCode } from '@opentelemetry/api';
import { DbStatementSerializer } from './types';

/**
* List of regexes and the number of arguments that should be serialized for matching commands.
* For example, HSET should serialize which key and field it's operating on, but not its value.
* Setting the subset to -1 will serialize all arguments.
* Commands without a match will have their first argument serialized.
*
* Refer to https://redis.io/commands/ for the full list.
*/
const SerializationSubsets = [
blumamir marked this conversation as resolved.
Show resolved Hide resolved
{
regex: /^(LPUSH|MSET|SET|PFA|PUBLISH|RPUSH|SADD|SET|SPUBLISH|XADD|ZADD)/i,
blumamir marked this conversation as resolved.
Show resolved Hide resolved
args: 1,
},
{
regex: /^(HSET|HMSET|LSET)/i,
args: 2,
},
{
regex: /^(LINSERT)/i,
args: 3,
},
{
regex:
/^(ACL|BIT|B[LRZ]|CLIENT|CLUSTER|CONFIG|COMMAND|DECR|DEL|EVAL|EX|FUNCTION|GEO|GET|HINCR|HMGET|HSCAN|INCR|L[TRLM]|MEMORY|P[EFISTU]|RPOP|S[CDIMORSU]|XACK|X[CDGILPRT]|Z[CDILMPRS])/i,
args: -1,
},
];

export const endSpan = (
span: Span,
err: NodeJS.ErrnoException | null | undefined
Expand All @@ -34,7 +62,16 @@ export const endSpan = (
export const defaultDbStatementSerializer: DbStatementSerializer = (
cmdName,
cmdArgs
) =>
Array.isArray(cmdArgs) && cmdArgs.length
? `${cmdName} ${cmdArgs.join(' ')}`
: cmdName;
) => {
if (Array.isArray(cmdArgs) && cmdArgs.length) {
const argsSubset =
SerializationSubsets.find(({ regex }) => {
return regex.test(cmdName);
})?.args || 1;
blumamir marked this conversation as resolved.
Show resolved Hide resolved
const args = argsSubset > 0 ? cmdArgs.slice(0, argsSubset) : cmdArgs;
blumamir marked this conversation as resolved.
Show resolved Hide resolved
return `${cmdName} ${args.join(' ')} [${
args.length !== cmdArgs.length ? cmdArgs.length - argsSubset : 0
blumamir marked this conversation as resolved.
Show resolved Hide resolved
} other arguments]`;
}
return cmdName;
};
23 changes: 14 additions & 9 deletions plugins/node/opentelemetry-instrumentation-ioredis/test/ioredis.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,19 +187,22 @@ describe('ioredis', () => {
description: string;
name: string;
args: Array<string>;
serializedArgs: Array<string>;
method: (cb: ioredisTypes.CallbackFunction<unknown>) => unknown;
}> = [
{
description: 'insert',
name: 'hset',
args: [hashKeyName, 'testField', 'testValue'],
serializedArgs: [hashKeyName, 'testField', '[1 other arguments]'],
method: (cb: ioredisTypes.CallbackFunction<number>) =>
client.hset(hashKeyName, 'testField', 'testValue', cb),
},
{
description: 'get',
name: 'get',
args: [testKeyName],
serializedArgs: [testKeyName, '[0 other arguments]'],
blumamir marked this conversation as resolved.
Show resolved Hide resolved
method: (cb: ioredisTypes.CallbackFunction<string | null>) =>
client.get(testKeyName, cb),
},
Expand Down Expand Up @@ -243,7 +246,7 @@ describe('ioredis', () => {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `${
command.name
} ${command.args.join(' ')}`,
} ${command.serializedArgs.join(' ')}`,
};
const span = provider
.getTracer('ioredis-test')
Expand Down Expand Up @@ -273,7 +276,7 @@ describe('ioredis', () => {
it('should create a child span for hset promise', async () => {
const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `hset ${hashKeyName} random random`,
[SemanticAttributes.DB_STATEMENT]: `hset ${hashKeyName} random [1 other arguments]`,
};
const span = provider.getTracer('ioredis-test').startSpan('test span');
await context.with(trace.setSpan(context.active(), span), async () => {
Expand Down Expand Up @@ -335,7 +338,8 @@ describe('ioredis', () => {
it('should create a child span for streamify scanning', done => {
const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: 'scan 0 MATCH test-* COUNT 1000',
[SemanticAttributes.DB_STATEMENT]:
'scan 0 MATCH test-* COUNT 1000 [0 other arguments]',
};
const span = provider.getTracer('ioredis-test').startSpan('test span');
context.with(trace.setSpan(context.active(), span), () => {
Expand Down Expand Up @@ -411,7 +415,8 @@ describe('ioredis', () => {

const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: 'subscribe news music',
[SemanticAttributes.DB_STATEMENT]:
'subscribe news music [0 other arguments]',
};
testUtils.assertSpan(
endedSpans[4],
Expand Down Expand Up @@ -466,7 +471,7 @@ describe('ioredis', () => {
it('should create a child span for pipeline', done => {
const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: 'set foo bar',
[SemanticAttributes.DB_STATEMENT]: 'set foo [1 other arguments]',
};

const span = provider.getTracer('ioredis-test').startSpan('test span');
Expand Down Expand Up @@ -500,7 +505,7 @@ describe('ioredis', () => {
it('should create a child span for get promise', async () => {
const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `get ${testKeyName}`,
[SemanticAttributes.DB_STATEMENT]: `get ${testKeyName} [0 other arguments]`,
};
const span = provider.getTracer('ioredis-test').startSpan('test span');
await context.with(trace.setSpan(context.active(), span), async () => {
Expand Down Expand Up @@ -529,7 +534,7 @@ describe('ioredis', () => {
it('should create a child span for del', async () => {
const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `del ${testKeyName}`,
[SemanticAttributes.DB_STATEMENT]: `del ${testKeyName} [0 other arguments]`,
};
const span = provider.getTracer('ioredis-test').startSpan('test span');
await context.with(trace.setSpan(context.active(), span), async () => {
Expand Down Expand Up @@ -563,7 +568,7 @@ describe('ioredis', () => {

const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `evalsha bfbf458525d6a0b19200bfd6db3af481156b367b 1 ${testKeyName}`,
[SemanticAttributes.DB_STATEMENT]: `evalsha bfbf458525d6a0b19200bfd6db3af481156b367b 1 ${testKeyName} [0 other arguments]`,
};

const span = provider.getTracer('ioredis-test').startSpan('test span');
Expand Down Expand Up @@ -663,7 +668,7 @@ describe('ioredis', () => {
SpanKind.CLIENT,
{
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: `set ${testKeyName} data`,
[SemanticAttributes.DB_STATEMENT]: `set ${testKeyName} [1 other arguments]`,
},
[],
unsetStatus
Expand Down