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 SQL detection #1427

Merged
merged 7 commits into from
Dec 3, 2024
Merged

Fix SQL detection #1427

merged 7 commits into from
Dec 3, 2024

Conversation

grcevski
Copy link
Contributor

@grcevski grcevski commented Dec 2, 2024

Our existing SQL heuristic was too lenient, we weren't detecting any specific protocols, but were simply searching for SQL text in the output. The simple heuristic is easy to defeat if we have a process that sends SQL like text in for example logs.

To fix this I added support for specific detection for MySQL and Postgres, while everything else will not be detected by default unless the user manually enables the SQL heuristic. This change doesn't affect Go, only protocol level detection.

While testing MySQL I also detected a few problems with HTTP calling to TLS enabled TCP requests. I've also fixed these in this PR. Mainly the issue was that related to SSL_write, where those Uprobes run before tcp_sendmsg. Therefore we were getting the connection information for the HTTP request and not the outgoing SSL write. This led to number of problems.

TODO:

  • Docs on the new option and the default behaviour

Closes #1422

Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 81.55340% with 38 lines in your changes missing coverage. Please review.

Project coverage is 80.89%. Comparing base (f517939) to head (ac119b9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/internal/ebpf/common/sql_detect_postgres.go 78.26% 19 Missing and 6 partials ⚠️
pkg/internal/ebpf/common/sql_detect_mysql.go 79.54% 6 Missing and 3 partials ⚠️
pkg/internal/ebpf/common/tcp_detect_transform.go 62.50% 2 Missing and 1 partial ⚠️
pkg/internal/request/span_getters.go 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1427      +/-   ##
==========================================
- Coverage   81.02%   80.89%   -0.14%     
==========================================
  Files         147      149       +2     
  Lines       15033    15117      +84     
==========================================
+ Hits        12181    12229      +48     
- Misses       2260     2288      +28     
- Partials      592      600       +8     
Flag Coverage Δ
integration-test 59.08% <30.09%> (-0.13%) ⬇️
k8s-integration-test 59.27% <1.94%> (-0.26%) ⬇️
oats-test 33.57% <34.46%> (-0.17%) ⬇️
unittests 51.75% <77.18%> (+0.14%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@grcevski grcevski merged commit 8f545d9 into grafana:main Dec 3, 2024
17 checks passed
@grcevski grcevski deleted the fix_sql_detection branch December 3, 2024 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLClient mistaken parses java logger messages as other_sql client spans
2 participants