-
Notifications
You must be signed in to change notification settings - Fork 612
feat(tedious): context propagation to SQL Server #3141
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
base: main
Are you sure you want to change the base?
Conversation
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
cfg.enableTraceContextPropagation && | ||
thisPlugin._shouldInjectFor(operation); | ||
|
||
if (!shouldInject) return runUserRequest(); |
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.
The previous code and this code path will return
the return value of the originalMethod()
call, which is typically what is wanted when wrapping. The _injectContextInfo()
code path below this will not. I gather that won't matter for this instrumentation because, at least currently, every patched method's return type is void
.
thisPlugin | ||
._injectContextInfo(this, tediousModule, traceparent) | ||
.finally(runUserRequest); |
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.
There is a lint error to deal with here:
286:9 error Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the `void` operator @typescript-eslint/no-floating-promises
I think you could do this (untested):
void thisPlugin
._injectContextInfo(this, tediousModule, traceparent)
.finally(runUserRequest);
thisPlugin | ||
._injectContextInfo(this, tediousModule, traceparent) | ||
.finally(runUserRequest); | ||
|
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.
Separate question on the async behaviour here:
- Without calling
set context_info
, a patchedconn.execSql()
will synchronously call the originalexecSql
method before returning. - With the changes here and when
set context_info
is enabled, that is no longer true. Now, the originalexecSql
will be called sometime later.
If I have JS code that does something like:
conn.execSql(sql1)
conn.execBulkLoad(sql2)
isn't there a sequencing problem here? I don't know tedious internals at all (e.g. whether and how it is queuing things). But with the example above, is it possible that the actual sequence of SQL executed is the following?
set context_info
sql2
sql1
(sc.traceFlags & api.TraceFlags.SAMPLED) === api.TraceFlags.SAMPLED | ||
? '01' | ||
: '00'; | ||
return `00-${sc.traceId}-${sc.spanId}-${sampled}`; |
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.
To be an absolutely correct traceparent
, with Trace Context Level 2, there are possibly other trace-flags to consider: https://www.w3.org/TR/trace-context-2/#trace-flags
I don't know if true, but I would hope there is some utility for creating the traceparent string from a SpanContext. Also, it looks like OTel JS doesn't yet implement the random-trace-id
flag. So this code is fine now, but could get surprised later.
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.
I didn't find a utility, but this building of the traceparent string is a little more future proof: https://github.com/open-telemetry/opentelemetry-js/blob/a2e3f4542f9a6a61c569d4f70fa49220da18cda0/packages/opentelemetry-core/src/trace/W3CTraceContextPropagator.ts#L84-L86
Perhaps use that handling of the traceFlags.
Calls
set context_info
before methods that actually execute user's SQL(omitting prepare and execBulkLoad). Relevant specification. This is opt in as it causes an extra round trip per request. .NET PR for the same thing