-
Notifications
You must be signed in to change notification settings - Fork 97
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
parse sql host address and port #1255
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1255 +/- ##
==========================================
- Coverage 77.27% 71.66% -5.62%
==========================================
Files 131 130 -1
Lines 13456 13456
==========================================
- Hits 10398 9643 -755
- Misses 2547 3174 +627
- Partials 511 639 +128
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I really like the idea, I think we are almost there. I pulled this branch and I'm going to take a look at what it looks like to add this. I think the main issue is that |
Hi @esara, I added a PR (on your branch) which should address the missing connection info and will work even if the SQL was done on another Go routine. esara#14. If it works for you, after you accept the changes we can work on an integration test. When I run this on the Go SQL test I added locally, it now shows something like:
|
hi @grcevski this change looks great, I tested it locally with a slightly larger go application and a postgres backend and it is correctly detecting the sql service name and port and populates it in the client span I am running more tests with the snowflake example, I mentioned in the issue later |
Hi Endre, I made a new PR with some integration tests esara#15. We've broken something so I made a draft PR so I can investigate the failrures myself. I'll push a new change to your PR once I know what's broken and close my PR. |
Hi Endre, I pushed new changes to my branch which is also used for my new PR against your branch and the tests passed: #1265. If you accept my PR changes as-is I think we should be in a good shape. I wonder if all these failures here might have to do with the version of clang/llvm you use for producing the BPF binaries? We are working towards switching to docker based builds for the BPF binaries to avoid this in the future. |
Added an integration test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks again @esara for your contribution!
parse sql host address and port for client spans to address #1254