feat(instrumentation-pg): propagate context using set application_name#3454
feat(instrumentation-pg): propagate context using set application_name#3454mhennoch wants to merge 11 commits intoopen-telemetry:mainfrom
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. |
|
I haven't checked the code yet, but can you add more tests? I can see that the codecov is failing, so this will need to increase the coverage |
|
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. |
|
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. |
|
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. |
raphael-theriault-swi
left a comment
There was a problem hiding this comment.
LGTM apart from the silent failure. Sorry it took so long to get a review.
| const traceparent = buildTraceparent(span); | ||
| if (traceparent) { | ||
| const setQuery = { | ||
| text: `SET application_name = '${traceparent}'`, | ||
| [INTERNAL_SET_QUERY]: true, | ||
| }; | ||
| try { | ||
| const setResult: unknown = original.apply(this, [ | ||
| setQuery, | ||
| ] as never); | ||
| if (setResult instanceof Promise) { | ||
| setResult.catch(() => {}); | ||
| } | ||
| } catch { | ||
| // Silently ignore SET failures to avoid breaking user queries | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd like there to be some diag warnings here on failure so it doesn't fail completely silently. Would make things a lot easier to debug for people if they're not getting correlation.
| return query; | ||
| } | ||
|
|
||
| const propagator = new W3CTraceContextPropagator(); |
There was a problem hiding this comment.
Nitpicking but this could be removed to use the newly added one at the module scope.
Changes
Adds SET application_name as a context propagation mechanism, following the pattern established for SQL Server (SET CONTEXT_INFO) and Oracle (V$SESSION.ACTION).
Why SET application_name?
PostgreSQL instrumentation already supports SQL Commenter, but it has limitations:
SET application_name operates at the session level and avoids all of these issues.
There is ongoing discussion in the npgsql community about this not being the ideal long-term solution, but no better alternative currently exists for PostgreSQL, and getting to one will take a long time. I will also create a semconv issue about it. This feature is disabled by default, as in the other instrumentations.
Existing and related implementations