SQL Server: recover DBM<>APM correlation comment for non-procedure statements - #24899
Open
pierreln-dd wants to merge 5 commits into
Open
SQL Server: recover DBM<>APM correlation comment for non-procedure statements#24899pierreln-dd wants to merge 5 commits into
pierreln-dd wants to merge 5 commits into
Conversation
…tements SQL Server's statement_start_offset/statement_end_offset exclude a leading comment (e.g. a sqlcommenter-style /*dddbs=...*/ tag) from statement_text, so DBM<>APM correlation comments were silently dropped for ordinary statements. The only place the comment could still be recovered from is row['text'] (the untouched batch text), but that re-obfuscation only ran when the statement had stored-procedure context. Run it for every row instead, keeping procedure_signature/procedure_name assignment gated behind has_proc_context as before. SDBM-2891
SDBM-2891
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 00d3676 | Docs | View more details | Give us feedback! |
Drop explanatory comment block, simplify changelog entry.
Address review feedback on #24899: - Only re-obfuscate row['text'] when it can add something (proc context, or text actually differs from statement_text), avoiding a doubled obfuscator RPC for every row where the two are already identical. - Extract the repeated "pull comments out of obfuscated metadata and merge" shape (full-text branch + tail-text branch) into _merge_comments, using dict.fromkeys instead of set for stable dd_comments ordering.
Contributor
Validation ReportAll 21 validations passed. Show details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes a bug in the SQL Server check's activity and query metrics collection where a DBM<>APM correlation comment (for example, a sqlcommenter-style
/*dddbs=...*/tag) prepended to a query is silently dropped for ordinary non-stored-procedure statements, breaking the "Calling Service" correlation panel.SQL Server's
statement_start_offset/statement_end_offsetDMV mechanics exclude the RPC parameter declaration and any leading comment fromstatement_text. The untouched batch text (row['text']) retains both, but the integration only re-obfuscated that text for stored-procedure context. This PR also inspects differing full text for ordinary statements and merges its comments. Procedure signatures, content, and names remain gated behind the existing procedure context.The Activity and Query Metrics unit tests reproduce the observed RPC shape,
(@P1 int)/*dddbs=...*/ SELECT ..., and assert that the correlation comment is recovered without adding procedure metadata.Motivation
Confirmed via direct reproduction with a real SQL Server engine, both a raw ad-hoc query and the actual
dd-trace-gov2 SDK, that this affects DBM<>APM correlation for SQL Server universally, not just OpenTelemetry-instrumented clients as originally reported. See SDBM-2891.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is merged