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

fix(plugin-ioredis): end span on response from the server and set span status according to response #239

Merged
merged 22 commits into from
Jan 28, 2021
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
Next Next commit
feat(plugin-ioredis): enahnce response handling
- fix: end span on response from server, so duration accurately include the server processing
    and networking time
- fix: set span status to error if server response with error
- feat: add responseHook to plugin config so user can register hook to add custom attributes to
    span when successful response arrive from server
  • Loading branch information
Amir Blum committed Nov 2, 2020
commit cbdbd418b15ec1c56cc494cb86e942a8e4c23465
2 changes: 1 addition & 1 deletion plugins/node/opentelemetry-plugin-ioredis/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"types": "build/src/index.d.ts",
"repository": "open-telemetry/opentelemetry-js",
"scripts": {
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts'",
"test": "nyc ts-mocha -p tsconfig.json 'test/**/*.test.ts' -g 'should call responseHook when set in config'",
"test:debug": "cross-env RUN_REDIS_TESTS_LOCAL=true ts-mocha --inspect-brk --no-timeouts -p tsconfig.json 'test/**/*.test.ts'",
"test:local": "cross-env RUN_REDIS_TESTS_LOCAL=true npm run test",
"tdd": "npm run test -- --watch-extensions ts --watch",
Expand Down
15 changes: 14 additions & 1 deletion plugins/node/opentelemetry-plugin-ioredis/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import type * as ioredisTypes from 'ioredis';
import { PluginConfig } from '@opentelemetry/api';
import { PluginConfig, Span } from '@opentelemetry/api';

export interface IoredisCommand {
reject: (err: Error) => void;
Expand All @@ -38,10 +38,23 @@ export type DbStatementSerializer = (
cmdArgs: IoredisCommand['args']
) => string;

export interface RedisResponseCustomAttributeFunction {
(
span: Span,
cmdName: IoredisCommand['name'],
cmdArgs: IoredisCommand['args'],
/* eslint-disable @typescript-eslint/no-explicit-any */
response: any
): void;
}

/**
* Options available for the IORedis Plugin (see [documentation](https://github.com/open-telemetry/opentelemetry-js/tree/master/packages/opentelemetry-plugin-ioredis#ioredis-plugin-options))
*/
export interface IoredisPluginConfig extends PluginConfig {
/** Custom serializer function for the db.statement tag */
dbStatementSerializer?: DbStatementSerializer;

/** Function for adding custom attributes on db response */
responseHook?: RedisResponseCustomAttributeFunction;
}
29 changes: 27 additions & 2 deletions plugins/node/opentelemetry-plugin-ioredis/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@ import {

const endSpan = (span: Span, err: NodeJS.ErrnoException | null | undefined) => {
if (err) {
let code = CanonicalCode.UNKNOWN;
if (err.message.startsWith('NOSCRIPT')) {
code = CanonicalCode.NOT_FOUND;
}

span.setStatus({
code: CanonicalCode.UNKNOWN,
code,
message: err.message,
});
} else {
Expand Down Expand Up @@ -111,7 +116,27 @@ export const traceSendCommand = (

try {
const result = original.apply(this, arguments);
endSpan(span, null);

const origResolve = cmd.resolve;
/* eslint-disable @typescript-eslint/no-explicit-any */
cmd.resolve = (result: any) => {
if (config?.responseHook) {
try {
config.responseHook(span, cmd.name, cmd.args, result);
} catch (ex) {
// we have nothing to do with exception from hook
}
}
endSpan(span, null);
origResolve(result);
};

const origReject = cmd.reject;
cmd.reject = (err: Error) => {
endSpan(span, err);
origReject(err);
};

return result;
} catch (error) {
endSpan(span, error);
Expand Down
56 changes: 50 additions & 6 deletions plugins/node/opentelemetry-plugin-ioredis/test/ioredis.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

import { CanonicalCode, context, SpanKind, Status } from '@opentelemetry/api';
import { CanonicalCode, context, Span, SpanKind, Status } from '@opentelemetry/api';
import { NoopLogger } from '@opentelemetry/core';
import { NodeTracerProvider } from '@opentelemetry/node';
import { AsyncHooksContextManager } from '@opentelemetry/context-async-hooks';
Expand Down Expand Up @@ -127,9 +127,11 @@ describe('ioredis', () => {
assert.strictEqual(endedSpans.length, 3);
assert.strictEqual(endedSpans[2].name, 'test span');

client.quit(done);
assert.strictEqual(endedSpans.length, 4);
assert.strictEqual(endedSpans[3].name, 'quit');
client.quit(() => {
assert.strictEqual(endedSpans.length, 4);
assert.strictEqual(endedSpans[3].name, 'quit');
done();
});
};
const errorHandler = (err: Error) => {
assert.ifError(err);
Expand Down Expand Up @@ -253,6 +255,23 @@ describe('ioredis', () => {
});
});

it('should set span with error when redis return reject', async () => {
const span = provider.getTracer('ioredis-test').startSpan('test span');
await provider.getTracer('ioredis-test').withSpan(span, async () => {
await client.set('non-int-key', 'no-int-value');
try {
// should throw 'ReplyError: ERR value is not an integer or out of range'
// because the value im the key is not numeric and we try to increment it
await client.incr('non-int-key');
} catch (ex) {
const endedSpans = memoryExporter.getFinishedSpans();
assert.strictEqual(endedSpans.length, 2);
// redis 'incr' operation failed with exception, so span should indicate it
assert.notStrictEqual(endedSpans[1].status.code, CanonicalCode.OK);
}
});
});

it('should create a child span for streamify scanning', done => {
const attributes = {
...DEFAULT_ATTRIBUTES,
Expand Down Expand Up @@ -312,10 +331,10 @@ describe('ioredis', () => {
const spanNames = [
'connect',
'connect',
'subscribe',
'info',
'info',
'subscribe',
'subscribe',
'publish',
'publish',
'unsubscribe',
Expand Down Expand Up @@ -378,7 +397,9 @@ describe('ioredis', () => {
SpanKind.CLIENT,
attributes,
[],
okStatus
{
code: CanonicalCode.NOT_FOUND,
}
);
testUtils.assertPropagation(endedSpans[0], span);
done();
Expand Down Expand Up @@ -574,6 +595,29 @@ describe('ioredis', () => {
});
});

describe('Instrumenting with a custom responseHook', () => {

before(() => {
plugin.disable();
const config: IoredisPluginConfig = {
responseHook: (span: Span, cmdName: string, cmdArgs: Array<string | Buffer | number>, response: any) => {
console.log(cmdName);
},
};
plugin.enable(ioredis, provider, new NoopLogger(), config);
});

it(`should call responseHook when set in config`, async () => {
const span = provider
.getTracer('ioredis-test')
.startSpan('test span');
await provider.getTracer('ioredis-test').withSpan(span, async () => {
await client.incr('new-key');
});
});

});

describe('Removing instrumentation', () => {
before(() => {
plugin.disable();
Expand Down