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

Closed
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
Prev Previous commit
feat(instrumentation-pg): get custom SQL Commenter attributes from ac…
…tive context
joelmukuthu committed Apr 3, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit be38c3e4a401d38205526652fed7d5ce93512e12
Original file line number Diff line number Diff line change
@@ -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];
Original file line number Diff line number Diff line change
@@ -47,7 +47,7 @@ function arrayStringifyHelper(arr: Array<unknown>): string {
return '[' + arr.toString() + ']';
}

type SqlCommenterAttributes = Record<string, string>;
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