-
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?
Changes from all commits
c93bd3a
b4f5f13
96122db
ad9881e
ce2698f
8118935
fc547a1
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 |
---|---|---|
|
@@ -40,6 +40,11 @@ import { PACKAGE_NAME, PACKAGE_VERSION } from './version'; | |
const CURRENT_DATABASE = Symbol( | ||
'opentelemetry.instrumentation-tedious.current-database' | ||
); | ||
|
||
export const INJECTED_CTX = Symbol( | ||
'opentelemetry.instrumentation-tedious.context-info-injected' | ||
); | ||
|
||
const PATCHED_METHODS = [ | ||
'callProcedure', | ||
'execSql', | ||
|
@@ -89,7 +94,7 @@ export class TediousInstrumentation extends InstrumentationBase<TediousInstrumen | |
this._wrap( | ||
ConnectionPrototype, | ||
method, | ||
this._patchQuery(method) as any | ||
this._patchQuery(method, moduleExports) as any | ||
); | ||
} | ||
|
||
|
@@ -127,11 +132,65 @@ export class TediousInstrumentation extends InstrumentationBase<TediousInstrumen | |
}; | ||
} | ||
|
||
private _patchQuery(operation: string) { | ||
private _buildTraceparent(span: api.Span): string { | ||
const sc = span.spanContext(); | ||
const sampled = | ||
(sc.traceFlags & api.TraceFlags.SAMPLED) === api.TraceFlags.SAMPLED | ||
? '01' | ||
: '00'; | ||
return `00-${sc.traceId}-${sc.spanId}-${sampled}`; | ||
} | ||
|
||
/** | ||
* Fire a one-off `SET CONTEXT_INFO @opentelemetry_traceparent` on the same | ||
* connection. Marks the request with INJECTED_CTX so our patch skips it. | ||
*/ | ||
private _injectContextInfo( | ||
connection: any, | ||
tediousModule: typeof tedious, | ||
traceparent: string | ||
): Promise<void> { | ||
return new Promise(resolve => { | ||
try { | ||
const sql = 'set context_info @opentelemetry_traceparent'; | ||
const req = new tediousModule.Request(sql, (_err: any) => { | ||
resolve(); | ||
}); | ||
Object.defineProperty(req, INJECTED_CTX, { value: true }); | ||
const buf = Buffer.from(traceparent, 'utf8'); | ||
req.addParameter( | ||
'opentelemetry_traceparent', | ||
(tediousModule as any).TYPES.VarBinary, | ||
buf, | ||
{ length: buf.length } | ||
); | ||
|
||
connection.execSql(req); | ||
} catch { | ||
resolve(); | ||
} | ||
}); | ||
} | ||
|
||
private _shouldInjectFor(operation: string): boolean { | ||
return ( | ||
operation === 'execSql' || | ||
operation === 'execSqlBatch' || | ||
operation === 'callProcedure' || | ||
operation === 'execute' | ||
); | ||
} | ||
|
||
private _patchQuery(operation: string, tediousModule: typeof tedious) { | ||
return (originalMethod: UnknownFunction): UnknownFunction => { | ||
const thisPlugin = this; | ||
|
||
function patchedMethod(this: ApproxConnection, request: ApproxRequest) { | ||
// Skip our own injected request | ||
if ((request as any)?.[INJECTED_CTX]) { | ||
return originalMethod.apply(this, arguments as unknown as any[]); | ||
} | ||
|
||
if (!(request instanceof EventEmitter)) { | ||
thisPlugin._diag.warn( | ||
`Unexpected invocation of patched ${operation} method. Span not recorded` | ||
|
@@ -207,12 +266,28 @@ export class TediousInstrumentation extends InstrumentationBase<TediousInstrumen | |
thisPlugin._diag.error('Expected request.callback to be a function'); | ||
} | ||
|
||
return api.context.with( | ||
api.trace.setSpan(api.context.active(), span), | ||
originalMethod, | ||
this, | ||
...arguments | ||
); | ||
const runUserRequest = () => { | ||
return api.context.with( | ||
api.trace.setSpan(api.context.active(), span), | ||
originalMethod, | ||
this, | ||
...arguments | ||
); | ||
}; | ||
|
||
const cfg = thisPlugin.getConfig(); | ||
const shouldInject = | ||
cfg.enableTraceContextPropagation && | ||
thisPlugin._shouldInjectFor(operation); | ||
|
||
if (!shouldInject) return runUserRequest(); | ||
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 previous code and this code path will |
||
|
||
const traceparent = thisPlugin._buildTraceparent(span); | ||
thisPlugin | ||
._injectContextInfo(this, tediousModule, traceparent) | ||
.finally(runUserRequest); | ||
Comment on lines
+286
to
+288
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. There is a lint error to deal with here:
I think you could do this (untested):
|
||
|
||
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. Separate question on the async behaviour here:
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?
|
||
return; | ||
} | ||
|
||
Object.defineProperty(patchedMethod, 'length', { | ||
|
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-flagsI 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.