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

fix(pg): Do not add SQLCommenter comments to prepared statements #2456

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raphael-theriault-swi
Copy link
Contributor

Which problem is this PR solving?

Currently the pg instrumentation will add SQLCommenter comments to any query it can, even prepared ones. In the case of prepared statements this usually results in the trace context within the comment being incorrect for all but the first instance of the query being run.

Short description of the changes

This adds a simple check to skip adding the comment for prepared statements. See https://node-postgres.com/features/queries#prepared-statements.

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.78%. Comparing base (dfb2dff) to head (bacaab4).
Report is 247 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2456      +/-   ##
==========================================
- Coverage   90.97%   90.78%   -0.20%     
==========================================
  Files         146      156      +10     
  Lines        7492     7716     +224     
  Branches     1502     1584      +82     
==========================================
+ Hits         6816     7005     +189     
- Misses        676      711      +35     
Files with missing lines Coverage Δ
...elemetry-instrumentation-pg/src/instrumentation.ts 90.22% <100.00%> (+2.25%) ⬆️

... and 75 files with indirect coverage changes

@maryliag
Copy link
Contributor

results in the trace context within the comment being incorrect for all but the first instance of the query being run

can you explain a little more on this? why the comment would be only valid for the first execution of the query? what makes the following executions different?
I imagine the difference in executions mean different values on the prepared statement, but I don't think the comments itself are about the values, so I'm not following why it would no longer be valid for future executions.

@raphael-theriault-swi
Copy link
Contributor Author

raphael-theriault-swi commented Oct 11, 2024

The comments contain the trace context at the time the query is created. So with prepared statements, when executing the query for the first time the comment will be created with the current trace context, but every subsequent execution will use that same trace context since the query is cached on the database side. So the comment now forever references a trace and span that have long stopped existing.

It makes a lot more sense when you look at how prepared statements actually work at the database level. It's actually a two step process, one PREPARE list (int, int) AS SELECT * FROM todos LIMIT $1 OFFSET $2 /* trace context */; step which caches the raw query without any parameter values, followed by any number of EXECUTE list(100, 0); which only include the cached name and argument values.

@maryliag
Copy link
Contributor

@pichlermarc is this consider a breaking change? Just want to make sure everything is properly flagged before giving any approvals

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants