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(instrumentation-pg): support custom SQL Commenter attributes #1454

Closed
wants to merge 2 commits into from
Closed

feat(instrumentation-pg): support custom SQL Commenter attributes #1454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2faeda
feat(instrumentation-pg): support custom SQL Commenter attributes
joelmukuthu Apr 3, 2023
be38c3e
feat(instrumentation-pg): get custom SQL Commenter attributes from ac…
joelmukuthu Apr 3, 2023
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
16 changes: 13 additions & 3 deletions plugins/node/opentelemetry-instrumentation-pg/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,18 +215,28 @@ export class PgInstrumentation extends InstrumentationBase {
// Modify query text w/ a tracing comment before invoking original for
// tracing, but only if args[0] has one of our expected shapes.
//
// TODO: remove the `as ...` casts below when the TS version is upgraded.
// TODO: remove the `as ...` casts on `arg0` below when the TS version is upgraded.
// Newer TS versions will use the result of firstArgIsQueryObjectWithText
// to properly narrow arg0, but TS 4.3.5 does not.
if (instrumentationConfig.addSqlCommenterCommentToQueries) {
const customSqlCommenterAttributes = context
.active()
.getValue(
Symbol('customSqlCommenterAttributes')
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about this bit, any suggestions are welcomed

Copy link
Member

Choose a reason for hiding this comment

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

This won't work. This names the symbol but it is still unique each time it is called. You would need to use Symbol.for in order to get the same symbol each time, but I think this approach loses some of the advantage of using a symbol.

Using context seems like a reasonable way to go about this but I haven't seen any other instrumentations do this in any language I think. I think the idea of using context to inject things into instrumentations has merit, but maybe the mechanism can be generalized to other instrumentations as well.

@open-telemetry/javascript-maintainers what would you think?

Copy link
Author

Choose a reason for hiding this comment

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

You would need to use Symbol.for in order to get the same symbol each time, but I think this approach loses some of the advantage of using a symbol.

You're quite right, I was forced to use a Symbol here because that's what the API expects. I think if we came up with a standard way of using the context to pass data to instrumentations, then there'd be a different API here.

I'm not sure I can offer much insight into what the API could look like but I'm keen to hear what the others think and very happy to implement what is agreed upon 🙌

) as utils.SqlCommenterAttributes;
args[0] = firstArgIsString
? utils.addSqlCommenterComment(span, arg0 as string)
? utils.addSqlCommenterComment(
span,
arg0 as string,
customSqlCommenterAttributes
)
: firstArgIsQueryObjectWithText
? {
...(arg0 as utils.ObjectWithText),
text: utils.addSqlCommenterComment(
span,
(arg0 as utils.ObjectWithText).text
(arg0 as utils.ObjectWithText).text,
customSqlCommenterAttributes
),
}
: args[0];
Expand Down
15 changes: 11 additions & 4 deletions plugins/node/opentelemetry-instrumentation-pg/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ function arrayStringifyHelper(arr: Array<unknown>): string {
return '[' + arr.toString() + ']';
}

export type SqlCommenterAttributes = Record<string, string>;
Copy link
Author

Choose a reason for hiding this comment

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

Would it be preferred that I list all the known attributes instead of the index signature? Or both?


/**
* Helper function to get a low cardinality span name from whatever info we have
* about the query.
Expand Down Expand Up @@ -284,7 +286,11 @@ function fixedEncodeURIComponent(str: string) {
);
}

export function addSqlCommenterComment(span: Span, query: string): string {
export function addSqlCommenterComment(
span: Span,
query: string,
customAttributes: SqlCommenterAttributes = {}
): string {
if (typeof query !== 'string' || query.length === 0) {
return query;
}
Expand All @@ -296,7 +302,7 @@ export function addSqlCommenterComment(span: Span, query: string): string {
}

const propagator = new W3CTraceContextPropagator();
const headers: { [key: string]: string } = {};
const headers: SqlCommenterAttributes = customAttributes;
propagator.inject(
trace.setSpan(ROOT_CONTEXT, span),
headers,
Expand All @@ -311,9 +317,10 @@ export function addSqlCommenterComment(span: Span, query: string): string {
}

const commentString = sortedKeys
.map(key => {
.map((key) => {
const encodedKey = fixedEncodeURIComponent(key);
const encodedValue = fixedEncodeURIComponent(headers[key]);
return `${key}='${encodedValue}'`;
return `${encodedKey}='${encodedValue}'`;
})
.join(',');

Expand Down
54 changes: 54 additions & 0 deletions plugins/node/opentelemetry-instrumentation-pg/test/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,5 +289,59 @@ describe('utils.ts', () => {
"SELECT * from FOO; /*traceparent='00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-01',tracestate='foo%3D%27bar%2Cbaz%3D%27qux%21%28%29%2A%27%2Chack%3D%27DROP%20TABLE'*/"
);
});

it('supports custom attributes', () => {
const spanContext: SpanContext = {
traceId: 'd4cda95b652f4a1592b449d5929fda1b',
spanId: '6e0c63257de34c92',
traceFlags: TraceFlags.SAMPLED,
};

const query = 'SELECT * from FOO;';
assert.strictEqual(
utils.addSqlCommenterComment(
trace.wrapSpanContext(spanContext),
query,
{ controller: 'foo', action: 'bar' }
),
"SELECT * from FOO; /*action='bar',controller='foo',traceparent='00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-01'*/"
);
});

it('escapes special characters in keys', () => {
const spanContext: SpanContext = {
traceId: 'd4cda95b652f4a1592b449d5929fda1b',
spanId: '6e0c63257de34c92',
traceFlags: TraceFlags.SAMPLED,
};

const query = 'SELECT * from FOO;';
assert.strictEqual(
utils.addSqlCommenterComment(
trace.wrapSpanContext(spanContext),
query,
{ "'DROP TABLE": 'foo' }
),
"SELECT * from FOO; /*%27DROP%20TABLE='foo',traceparent='00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-01'*/"
);
});

it('overwrites custom attributes with trace context keys', () => {
const spanContext: SpanContext = {
traceId: 'd4cda95b652f4a1592b449d5929fda1b',
spanId: '6e0c63257de34c92',
traceFlags: TraceFlags.SAMPLED,
};

const query = 'SELECT * from FOO;';
assert.strictEqual(
utils.addSqlCommenterComment(
trace.wrapSpanContext(spanContext),
query,
{ traceparent: 'foo' }
),
"SELECT * from FOO; /*traceparent='00-d4cda95b652f4a1592b449d5929fda1b-6e0c63257de34c92-01'*/"
);
});
});
});