fix(pkg/instrumentation/databasesql): traces drop server.address for standard DSN formats #470
fix(pkg/instrumentation/databasesql): traces drop server.address for standard DSN formats #470alaotach wants to merge 5 commits into
Conversation
…standard DSN formats
|
|
There was a problem hiding this comment.
Pull request overview
This pull request improves pkg/instrumentation/databasesql DSN parsing so spans retain server.address, server.port, and db.namespace for common, valid DSN formats that previously failed parsing and caused attribute fallback to "unknown" / empty namespace.
Changes:
- Extend Postgres parsing to support libpq key/value DSNs (
host=... port=... dbname=...) with a default port fallback. - Extend MySQL parsing to treat DSNs without an explicit address (
user:pass@/dbnameandtcp()/...) aslocalhost:3306. - Enhance db name extraction to detect
dbname=/database=key/value patterns before falling back to path-based parsing, and add integration tests + test driver aliases to cover these cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
pkg/instrumentation/databasesql/parse.go |
Adds key/value dbname parsing and expands Postgres/MySQL DSN parsing fallbacks to preserve endpoint + namespace attributes. |
test/integration/db_client_test.go |
Adds integration test coverage for Postgres libpq DSNs, MySQL default-host DSNs, and SQL Server Database= key DSNs. |
test/apps/dbclient/main.go |
Registers additional driver-name aliases in the integration test app so the new DSN parsing paths are exercised. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !strings.Contains(url, "://") { | ||
| host, port, ok := parseConnectionStringPair(url, []string{"host"}) | ||
| if !ok || host == "" { | ||
| return "", errors.New("invalid Postgres DSN") | ||
| } | ||
| if port == "" { | ||
| port = "5432" | ||
| } | ||
| return host + ":" + port, nil | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #470 +/- ##
=========================================
+ Coverage 0 62.73% +62.73%
=========================================
Files 0 62 +62
Lines 0 4760 +4760
=========================================
+ Hits 0 2986 +2986
- Misses 0 1529 +1529
- Partials 0 245 +245
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:
|
Use JoinHostPort for libpq key/value hosts and add IPv6 DSN coverage. Reuse the dbclient build across DB integration tests to cut runtime.
|
hey I’ve fixed the issues and the libpq host parsing problem, the pr should be safe to merge. please take a look and let me know if you want any changes |
|
@NameHaibinZhang Please help review this when you have time. Thanks. |
| "strings" | ||
| ) | ||
|
|
||
| var dbNamePattern = regexp.MustCompile(`(?i)(^|[\s;?&])(?:dbname|database)\s*=\s*('([^']*)'|"([^"]*)"|[^;\s&]+)`) |
There was a problem hiding this comment.
This regex is too tricky, e.g. from user:pass@tcp(host)/mydb?timeout=30s&database=other matches database=other
what about making parseDSN return also the dbname?
|
|
||
| func init() { | ||
| sql.Register("testdb", &testDriver{}) | ||
| sql.Register("mysql", &testDriver{}) |
There was a problem hiding this comment.
What's the value of these new registrations in the test?
|
|
||
| var dbNamePattern = regexp.MustCompile(`(?i)(^|[\s;?&])(?:dbname|database)\s*=\s*('([^']*)'|"([^"]*)"|[^;\s&]+)`) | ||
|
|
||
| func ParseDbName(dsn string) string { |
There was a problem hiding this comment.
Could we add some unit tests to cover non-happy path scenarios?
The DSN parsing in parse.go didn't handle several standard connection string formats causing spans to silently lose server.address, port, and namespace attributes for perfectly valid DSNs
What changed:
Integration tests added for all three cases following existing test structure. Test app driver registration extended to cover the aliases these tests need (mysql, postgres, sqlserver etc).
Before this, any app using these DSN formats would get server.address="unknown" and empty namespace in their traces which defeats the point of the instrumentation.
closes #460