-
Notifications
You must be signed in to change notification settings - Fork 534
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: add sqlcommenter comment with trace context to queries in pg instrumentation #1286
Changes from 5 commits
cb27d60
2425c57
41e49f1
08a9e0c
d53b355
6e3302c
f296325
b066ecc
b79ab31
a7e76e4
dfec605
1f0ac72
16a45fa
18c6dbe
486b903
d37e50c
9a11712
f85c645
70ac1cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,10 @@ import { | |
diag, | ||
INVALID_SPAN_CONTEXT, | ||
Attributes, | ||
defaultTextMapSetter, | ||
ROOT_CONTEXT, | ||
} from '@opentelemetry/api'; | ||
import { W3CTraceContextPropagator } from '@opentelemetry/core'; | ||
import { AttributeNames } from './enums/AttributeNames'; | ||
import { | ||
SemanticAttributes, | ||
|
@@ -289,3 +292,60 @@ export function patchClientConnectCallback( | |
cb.call(this, err); | ||
}; | ||
} | ||
|
||
function hasValidSqlComment(query: string): boolean { | ||
const indexOpeningDashDashComment = query.indexOf('--'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The query might contain "--" as part of parameter. In that case, this method will return true that the query contains a comment. For Ex:
I am not sure how to detect correctly that a query contains a comment without using a parser. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the same also apply for the block comment? i.e if it is inside of a string literal, it is not a comment? I took the current code almost verbatim from the existing sqlcommenter Node implementation (the knex one for example), so the fault is present there as well. Adding a parser would not really be a problem, I think, although not sure what the overhead would be for each query execution, perhaps a comment in the source code + a OpenTelemetry diagnostic log line for queries that are ignored is a nice starting point? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it might affect a block comment also. I agree the issue originates in the original SQLCommenter libraries. That's why i am inclined towards adding a comment regarding this issue here (and if possible callout in docs). Adding a parser might affect the performance as well as will be an additional dependency in this library (which databases are anyway doing). We are thinking if the specs regarding "do not add a comment if there is already a comment" could be modified. Should it just be "add a new comment at the end of the statement" ?. So, maybe till that is finalised, we can add a comment and tech debt maybe ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment and a short note to the README for the flag. |
||
if (indexOpeningDashDashComment >= 0) { | ||
return true; | ||
} | ||
|
||
const indexOpeningSlashComment = query.indexOf('/*'); | ||
if (indexOpeningSlashComment < 0) { | ||
return false; | ||
} | ||
|
||
const indexClosingSlashComment = query.indexOf('*/'); | ||
return indexOpeningDashDashComment < indexClosingSlashComment; | ||
} | ||
|
||
export function addSqlCommenterComment(span: Span, query: string): string { | ||
if (query.length === 0) { | ||
henrinormak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return query; | ||
} | ||
|
||
// As per sqlcommenter spec we shall not add a comment if there already is a comment | ||
// in the query | ||
if (hasValidSqlComment(query)) { | ||
return query; | ||
} | ||
|
||
const propagator = new W3CTraceContextPropagator(); | ||
const headers: { [key: string]: string } = {}; | ||
propagator.inject( | ||
trace.setSpan(ROOT_CONTEXT, span), | ||
headers, | ||
defaultTextMapSetter | ||
Flarna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
); | ||
|
||
// sqlcommenter spec requires keys in the comment to be sorted lexicographically | ||
const sortedKeys = Object.keys(headers).sort(); | ||
|
||
if (sortedKeys.length === 0) { | ||
return query; | ||
} | ||
|
||
const commentString = sortedKeys | ||
.map(key => { | ||
const uriEncodedKey = encodeURIComponent(key); | ||
henrinormak marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const uriEncodedValue = encodeURIComponent(headers[key]).replace( | ||
/([^\\])(')/g, | ||
(_, prefix) => { | ||
return `${prefix}\\'`; | ||
} | ||
); | ||
return `${uriEncodedKey}='${uriEncodedValue}'`; | ||
}) | ||
.join(','); | ||
|
||
return `${query} /*${commentString}*/`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was previously added, but README was out of date.