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(instrumentation-pg): add post-query custom attributes hook for spans #767

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions plugins/node/opentelemetry-instrumentation-pg/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ PostgreSQL instrumentation has few options available to choose from. You can set
| Options | Type | Description |
| ------- | ---- | ----------- |
| [`enhancedDatabaseReporting`](https:/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-pg/src/pg.ts#L48) | `boolean` | If true, additional information about query parameters and results will be attached (as `attributes`) to spans representing database operations |
| `postQueryHook` | `PgPostQueryHookFunction` (function) | Function for adding custom attributes before the query is resolved |
| `responseHook` | `PgInstrumentationExecutionResponseHook` (function) | Function for adding custom attributes from db response |

## Useful links
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
PgPoolExtended,
PgPoolCallback,
PgInstrumentationConfig,
QueryContext,
} from './types';
import * as utils from './utils';
import { AttributeNames } from './enums/AttributeNames';
Expand Down Expand Up @@ -114,7 +115,9 @@ export class PgInstrumentation extends InstrumentationBase {
`Patching ${PgInstrumentation.COMPONENT}.Client.prototype.query`
);
return function query(this: PgClientExtended, ...args: unknown[]) {
const pluginConfig = plugin.getConfig() as PgInstrumentationConfig;
let span: Span;
let postQueryHookParams: QueryContext;

// Handle different client.query(...) signatures
if (typeof args[0] === 'string') {
Expand All @@ -124,21 +127,24 @@ export class PgInstrumentation extends InstrumentationBase {
span = utils.handleParameterizedQuery.call(
this,
plugin.tracer,
plugin.getConfig() as PgInstrumentationConfig,
pluginConfig,
query,
params
);
postQueryHookParams = { span, query, params };
} else {
span = utils.handleTextQuery.call(this, plugin.tracer, query);
postQueryHookParams = { span, query };
}
} else if (typeof args[0] === 'object') {
const queryConfig = args[0] as NormalizedQueryConfig;
span = utils.handleConfigQuery.call(
this,
plugin.tracer,
plugin.getConfig() as PgInstrumentationConfig,
pluginConfig,
queryConfig
);
postQueryHookParams = { span, config: queryConfig };
} else {
return utils.handleInvalidQuery.call(
this,
Expand All @@ -148,13 +154,15 @@ export class PgInstrumentation extends InstrumentationBase {
);
}

utils.handlePostQueryHook(pluginConfig, postQueryHookParams);

// Bind callback to parent span
if (args.length > 0) {
const parentSpan = trace.getSpan(context.active());
if (typeof args[args.length - 1] === 'function') {
// Patch ParameterQuery callback
args[args.length - 1] = utils.patchCallback(
plugin.getConfig() as PgInstrumentationConfig,
pluginConfig,
span,
args[args.length - 1] as PostgresCallback
);
Expand All @@ -170,7 +178,7 @@ export class PgInstrumentation extends InstrumentationBase {
) {
// Patch ConfigQuery callback
let callback = utils.patchCallback(
plugin.getConfig() as PgInstrumentationConfig,
pluginConfig,
span,
(args[0] as NormalizedQueryConfig).callback!
);
Expand All @@ -194,11 +202,7 @@ export class PgInstrumentation extends InstrumentationBase {
.then((result: unknown) => {
// Return a pass-along promise which ends the span and then goes to user's orig resolvers
return new Promise(resolve => {
utils.handleExecutionResult(
plugin.getConfig() as PgInstrumentationConfig,
span,
result
);
utils.handleExecutionResult(pluginConfig, span, result);
span.end();
resolve(result);
});
Expand Down
16 changes: 16 additions & 0 deletions plugins/node/opentelemetry-instrumentation-pg/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,28 @@ export interface PgInstrumentationExecutionResponseHook {
(span: api.Span, responseInfo: PgResponseHookInformation): void;
}

export interface QueryContext {
span: api.Span;
query?: string;
config?: NormalizedQueryConfig;
params?: unknown[];
}

export interface PgPostQueryHookFunction {
(ctx: QueryContext): void;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please change the hook signature to follow the other hook example in this file:

export interface PgInstrumentationExecutionResponseHook {
  (span: api.Span, responseInfo: PgResponseHookInformation): void;
}

First parameter should be the span.
Second parameter is called Info on the other hook. Could be nice in my opinion to call it queryInfo (to stay consistent with the existing name.
From what I remember, this is also the convention in other instrumentations.

}

export interface PgInstrumentationConfig extends InstrumentationConfig {
/**
* If true, additional information about query parameters will be attached (as `attributes`) to spans representing
*/
enhancedDatabaseReporting?: boolean;

/**
* Function for adding custom attributes before the query is resolved
*/
postQueryHook?: PgPostQueryHookFunction;

/**
* Hook that allows adding custom span attributes based on the data
* returned from "query" Pg actions.
Expand Down
20 changes: 20 additions & 0 deletions plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
PgPoolCallback,
PgPoolExtended,
PgInstrumentationConfig,
QueryContext,
} from './types';
import * as pgTypes from 'pg';
import { PgInstrumentation } from './';
Expand Down Expand Up @@ -169,6 +170,25 @@ export function handleInvalidQuery(
return result;
}

export function handlePostQueryHook(
config: PgInstrumentationConfig,
ctx: QueryContext
) {
if (typeof config.postQueryHook === 'function') {
safeExecuteInTheMiddle(
() => {
config.postQueryHook!(ctx);
},
err => {
if (err) {
diag.error('Error running post query hook', err);
}
},
true
);
}
}

export function handleExecutionResult(
config: PgInstrumentationConfig,
span: Span,
Expand Down
108 changes: 108 additions & 0 deletions plugins/node/opentelemetry-instrumentation-pg/test/pg.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -563,5 +563,113 @@ describe('pg', () => {
client.query('SELECT NOW()').then(queryHandler);
});
});

it('should call postQueryHook with query text if set', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

You can wrap this section with a describe to group all these tests under postQueryHook.
This will also be consistent with the existing tests for responseHook see here

instrumentation.disable();
let called = false;
const query = 'SELECT NOW()';
const config: PgInstrumentationConfig = {
postQueryHook: ctx => {
called = true;
assert.strictEqual(ctx.query, query);
assert.strictEqual(ctx.params, undefined);
Comment on lines +574 to +575
Copy link
Member

Choose a reason for hiding this comment

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

These assertions are performed in the hook body.
I think that if they fail, the exception will just be silently ignored and test will pass.
That is because the hook is protected with safeExecuteInTheMiddle which does not propagate exceptions to the caller.

},
};
instrumentation.setConfig(config);
instrumentation.enable();

const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: query,
};
const events: TimedEvent[] = [];
const span = tracer.startSpan('test span');
await context.with(trace.setSpan(context.active(), span), async () => {
try {
const resPromise = await client.query(query);
assert.ok(resPromise);
runCallbackTest(span, attributes, events);
} catch (e) {
assert.ok(false, e.message);
}
});
assert.strictEqual(called, true);
});

it('should call postQueryHook with query text and params if set', async () => {
instrumentation.disable();
let called = false;
const values = ['0'];
const query = 'SELECT $1::text';
const config: PgInstrumentationConfig = {
postQueryHook: ctx => {
called = true;
assert.strictEqual(ctx.query, query);
assert.strictEqual(ctx.params, values);
},
};
instrumentation.setConfig(config);
instrumentation.enable();

const attributes = {
...DEFAULT_ATTRIBUTES,
[SemanticAttributes.DB_STATEMENT]: query,
};
const events: TimedEvent[] = [];
const span = tracer.startSpan('test span');
await context.with(trace.setSpan(context.active(), span), async () => {
const resPromise = await client.query(query, values);
try {
assert.ok(resPromise);
runCallbackTest(span, attributes, events);
} catch (e) {
assert.ok(false, e.message);
}
});
assert.strictEqual(called, true);
});

it('should call postQueryHook with query config if set', async () => {
instrumentation.disable();
const name = 'fetch-text';
const query = 'SELECT $1::text';
const values = ['0'];
let called = false;
const config: PgInstrumentationConfig = {
postQueryHook: ctx => {
called = true;
if (!ctx.config) {
assert.ok(false, 'ctx.config was undefined');
}
assert.strictEqual(ctx.config.text, query);
assert.strictEqual(ctx.config.values, values);
},
};
instrumentation.setConfig(config);
instrumentation.enable();

const attributes = {
...DEFAULT_ATTRIBUTES,
[AttributeNames.PG_PLAN]: name,
[SemanticAttributes.DB_STATEMENT]: query,
};
const events: TimedEvent[] = [];
const span = tracer.startSpan('test span');

await context.with(trace.setSpan(context.active(), span), async () => {
try {
const resPromise = await client.query({
name: name,
text: query,
values: values,
});
assert.strictEqual(resPromise.command, 'SELECT');
runCallbackTest(span, attributes, events);
} catch (e) {
assert.ok(false, e.message);
}
});
assert.strictEqual(called, true);
});
Copy link
Member

Choose a reason for hiding this comment

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

I also suggest having some negative tests for the hook - what happens if it throws? what if it's not a function?
This introduced new logic is currently not covered with tests.
You can see an example of that on the responseHook tests here

});
});