[SqlClient] Recognise more keywords#3671
Conversation
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
github-actions
Bot
added
infra
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3671 +/- ##
==========================================
+ Coverage 71.46% 71.62% +0.16%
==========================================
Files 455 455
Lines 17654 17700 +46
==========================================
+ Hits 12616 12678 +62
+ Misses 5038 5022 -16 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
github-actions
Bot
added
comp:instrumentation.entityframeworkcore
|
/cc @stevejgordon |
Recognise more SQL keywords to improve query summaries. Resolves open-telemetry#3666.
martincostello
force-pushed
the
gh-3666
branch
from
8627078 to
4b438b7
Compare
martincostello
had a problem deploying
to
external
GitHub Actions
Failure
github-actions
Bot
removed
infra
There was a problem hiding this comment.
Pull request overview
This pull request enhances SQL parsing capabilities by adding recognition for additional T-SQL keywords to improve query summary generation. The changes address issue #3666 by implementing support for commands like GRANT, DENY, REVOKE, TRUNCATE, BACKUP, RESTORE, BULK INSERT, and ENABLE/DISABLE TRIGGER.
Key changes:
- Added 12 new SQL keywords (BACKUP, BULK, CONNECT, DENY, DISABLE, ENABLE, EXECUTE, GRANT, RESTORE, REVOKE, STATISTICS, TRUNCATE) to the SQL processor
- Updated keyword relationship mappings to properly handle multi-word SQL commands
- Modified existing test cases to reflect new expected behavior (e.g., "REVOKE SELECT" now summarizes as "REVOKE")
- Added comprehensive test coverage for all new keywords
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Shared/SqlProcessor.cs | Added new SQL keyword enums, keyword info instances, and configured keyword relationships and capture logic for T-SQL statement recognition |
| test/OpenTelemetry.Contrib.Shared.Tests/SqlProcessorAdditionalTestCases.json | Updated existing test cases to reflect new summary behavior and added 7 new test cases for newly supported keywords |
| src/OpenTelemetry.Instrumentation.SqlClient/CHANGELOG.md | Documented the improvement to SQL parsing for T-SQL keywords |
| src/OpenTelemetry.Instrumentation.EntityFrameworkCore/CHANGELOG.md | Documented the improvement to SQL parsing for T-SQL keywords |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alanwest
left a comment
alanwest
left a comment
There was a problem hiding this comment.
Approving, but just sharing the observation that even prior to this PR we appear to have some inconsistency for when we capture the target of an operation in the db.query.summary.
I don't feel strongly that anything needs to be addressed in this PR, but we should probably discuss our general stance for when/why we include a target in db.query.summary.
| "db.query.text": [ | ||
| "DISABLE TRIGGER Person.uAddress ON Person.Address;" | ||
| ], | ||
| "db.query.summary": "DISABLE TRIGGER Person.uAddress" |
There was a problem hiding this comment.
Sometimes db.query.summary contains the target of the operation.
| "RESTORE DATABASE AdventureWorks2022 FROM AdventureWorks2022Backups;" | ||
| ], | ||
| "db.query.summary": "RESTORE DATABASE" |
There was a problem hiding this comment.
Yeah, I got a bit bogged down in exactly how to configure things in the code to get it to capture things or not.
Rather than sink too much time into it (or breaking something) I just figured incremental improvement was best for now.
b6723bb
Fixes #3666
Changes
Recognise more SQL keywords to improve query summaries.
I used the Transact-SQL statements documentation as a starting point, and went through any keywords not already present that were either a unique new first keyword (e.g.
GRANT) or were a unique secondary keyword (e.g.STATISTICS).I'm open to adding/removing the keywords or tweaking the summaries generated.
Depends on #3660.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)