Conversation
|
|
NimrodAvni78
left a comment
There was a problem hiding this comment.
This is great, thanks!
would love to see some integration tests for this, im pretty sure there are some open source images for mssql we can use
i think after pushing a new commit tests will also start running
also adding copilot for review
There was a problem hiding this comment.
Pull request overview
Adds Microsoft SQL Server (MSSQL/TDS) support to OBI’s eBPF-based TCP/SQL instrumentation so MSSQL requests can be detected, parsed (including prepared statement flows), and emitted as SQL client spans.
Changes:
- Add kernel-space (eBPF) protocol detection and large-buffer capture support for MSSQL/TDS.
- Add user-space MSSQL SQL detection/parsing (batch + RPC prepare/execute caching) and wire it into the TCP span pipeline.
- Extend configuration/defaults and documentation to expose MSSQL buffer sizing + prepared statement cache sizing.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/obi/config_test.go | Updates default-config expectations to include MSSQL prepared statement cache size. |
| pkg/obi/config.go | Adds MSSQL defaults (buffer size + prepared statement cache size). |
| pkg/internal/sqlprune/sqlparser.go | Routes SQL command/error parsing to MSSQL-specific parser. |
| pkg/internal/sqlprune/mssql.go | Implements basic MSSQL/TDS response error parsing + command ID mapping. |
| pkg/internal/ebpf/generictracer/generictracer.go | Exposes mssql_buffer_size constant to eBPF program. |
| pkg/ebpf/common/tcp_detect_transform.go | Adds MSSQL protocol-type handler to produce SQL spans (or fallback). |
| pkg/ebpf/common/sql_detect_transform.go | Adds MSSQL detection to SQL kind heuristics + prepared statement parsing hook. |
| pkg/ebpf/common/sql_detect_mssql.go | Implements MSSQL/TDS header validation, UCS-2 decoding, RPC parsing, and prepared-stmt handle caching. |
| pkg/ebpf/common/sql_detect_mssql_test.go | Adds unit tests for MSSQL detection and parsing helpers. |
| pkg/ebpf/common/common.go | Adds MSSQL protocol enum + parse-context LRU for MSSQL prepared statements. |
| pkg/config/ebpf_tracer.go | Adds MSSQL prepared statement cache size + MSSQL buffer size config knobs. |
| pkg/appolly/app/request/span.go | Adds DBMSSQL kind and maps it to OTel db.system semantic convention. |
| devdocs/features.md | Documents MSSQL support in feature matrix. |
| bpf/generictracer/protocol_tcp.h | Enables MSSQL large-buffer emission in TCP protocol handler. |
| bpf/generictracer/protocol_mssql.h | Adds MSSQL/TDS protocol detection + large-buffer emission implementation. |
| bpf/generictracer/k_tracer.c | Adds MSSQL detection in kernel-side protocol classification pipeline. |
| bpf/common/large_buffers.h | Adds MSSQL buffer-size constant and scratch buffer. |
| bpf/common/connection_info.h | Adds MSSQL protocol enum value for kernel/user alignment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
@NimrodAvni78 I tried searching There is an official image for mssql: Would using all of that, and adding a test, similar to how the |
rafaelroquetto
left a comment
There was a problem hiding this comment.
Good stuff! Thank you so much for spearheading this!
If possible, avoid materialising a byte slice and instead use the large buffer API directly - or, I see that some of the functions here are a perfect fit for the large buffer Reader (cursor) API - using those prevents us from having to allocate a big scratch chunk and instead will consume the bytes mostly in-place from the original backing store.
Also, this will conflict with: #1513 - since that PR is already approved, could you wait until that's in and rebase on top of it?
@rafaelroquetto Thanks :) |
yeah something like this, add an example service testing different scenarios (in my head the main ones are plain query, error query and prepared statement), and testing it the same as other protocols like mysql |
Should I wait for #1539 so I don't cause conflicts? |
mmat11
left a comment
There was a problem hiding this comment.
left a couple nits, lgtm!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
+ Coverage 67.22% 69.19% +1.97%
==========================================
Files 277 279 +2
Lines 33258 33594 +336
==========================================
+ Hits 22358 23246 +888
+ Misses 9539 9092 -447
+ Partials 1361 1256 -105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Don't think so, that will only effect mysql test suite |
…some checks in isMSSQL
|
I merged origin/main to my branch to deal with the conflicts. it should be good to go :) |
CI Supervisor: Pull request checks (attempt 2)
|
rafaelroquetto
left a comment
There was a problem hiding this comment.
This looks great, there's just one major last change we should do - decouple the eom detection from mssql_send_large_buffer, as that's completely orthogonal and does not belong there (unless I am missing something).
Otherwise, some minor feedback:
- missing default for
MSSQLPreparedStatementsCacheSize- since the field has validate:"gt=0", OBI will fail validation on startup unless the user explicitly sets it - mssqlPreparedStatements function is misnamed: it handles SQL_BATCH plain queries, not prepared statements. RPC/prepared-statement handling is in handleMSSQL directly. Name it mssqlExtractBatchSQL or similar.
- improvements to multi-packet test: testPythonSQLMultiPacketResponse only asserts a SELECT span appeared, it doesn't verify the SQL text was correctly reassembled. The whole point of the /largeresult endpoint and bulk_actor table is to force multi-packet reassembly, but the assertion doesn't confirm it worked.
- BPF/Go detection divergence: BPF is_mssql accepts login7/prelogin packets; Go isMSSQL does not. Not a bug (login packets don't carry SQL), but undocumented and will confuse anyone trying to understand why the two functions differ.
(these bullet points were generated with the help of Claude).
The PR looks good overall, I am marking this with "request changes" just to prevent it from accidentally being merged since it's already gotten an approval.
| } | ||
| } | ||
|
|
||
| if (packet_type == PACKET_TYPE_RESPONSE) { |
There was a problem hiding this comment.
I am not sure if this is the best place for this - this seems to be detecting the end of a message, even when large buffers are not enabled for this protocol (i.e. mssql_max_captured_byted == 0). In this scenario, no buffer will ever be sent but yet this function is performing unrelated work for every packet.
Maybe it would be cleaner to have someething like mssql_respnse_complete() instead, to be called from handle_unknown_tcp_connection() alongside the large buffer call, as it is somewhat unrelated.
This enables instrumentation of requests and queries to Microsoft SQL Server (mssql) from OBI services. Implemented similarly to how MySQL and PostgreSQL instrumentation are implemented.
Resolve #1285
Checklist