fix user & host parsing in SQL connection string#8431
Merged
Conversation
andrewlock
approved these changes
Apr 9, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Thanks - just a question about where the spec for these lives, so we can be sure that we're not missing anything else?
| return new TagsCacheItem( | ||
| dbName: GetConnectionStringValue(builder, "Database", "Initial Catalog", "InitialCatalog"), | ||
| dbUser: GetConnectionStringValue(builder, "User ID", "UserID"), | ||
| dbUser: GetConnectionStringValue(builder, "User ID", "UserID", "User", "Uid", "Username", "User Name"), |
Member
There was a problem hiding this comment.
Is there a spec for this, so we can be sure we're not missing anything anywhere else?
Contributor
Author
There was a problem hiding this comment.
no spec, I asked the AI to make really sure nothing else was missing and it said we're all good, so it's 100% sure
Member
There was a problem hiding this comment.
no spec
:eyetwitch: :stare:
This feels like something that really should have a cross-tracer spec and cross tracer tests right, seeing as it's database specific, not language specific?
Contributor
Author
There was a problem hiding this comment.
I edited the description for db.user in the confluence page, good enough as a spec
(thanks andrew for the pointer)
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8431) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (72ms) : 68, 75
master - mean (71ms) : 69, 73
section Bailout
This PR (8431) - mean (75ms) : 73, 77
master - mean (76ms) : 73, 79
section CallTarget+Inlining+NGEN
This PR (8431) - mean (1,063ms) : 1027, 1099
master - mean (1,062ms) : 1019, 1105
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (113ms) : 110, 116
master - mean (112ms) : 109, 116
section Bailout
This PR (8431) - mean (113ms) : 110, 116
master - mean (114ms) : 112, 117
section CallTarget+Inlining+NGEN
This PR (8431) - mean (783ms) : 760, 805
master - mean (784ms) : 761, 807
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (100ms) : 97, 103
master - mean (100ms) : 96, 103
section Bailout
This PR (8431) - mean (101ms) : 98, 104
master - mean (100ms) : 97, 102
section CallTarget+Inlining+NGEN
This PR (8431) - mean (930ms) : 902, 959
master - mean (930ms) : 890, 971
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (98ms) : 95, 101
master - mean (98ms) : 94, 102
section Bailout
This PR (8431) - mean (99ms) : 97, 101
master - mean (99ms) : 96, 103
section CallTarget+Inlining+NGEN
This PR (8431) - mean (816ms) : 776, 857
master - mean (819ms) : 780, 859
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (211ms) : 206, 216
master - mean (206ms) : 201, 211
section Bailout
This PR (8431) - mean (215ms) : 211, 219
master - mean (210ms) : 206, 213
section CallTarget+Inlining+NGEN
This PR (8431) - mean (1,238ms) : 1181, 1296
master - mean (1,207ms) : 1169, 1246
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (305ms) : 298, 312
master - mean (297ms) : 289, 305
section Bailout
This PR (8431) - mean (306ms) : 300, 311
master - mean (299ms) : 293, 306
section CallTarget+Inlining+NGEN
This PR (8431) - mean (1,016ms) : 984, 1047
master - mean (1,002ms) : 964, 1040
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (299ms) : 293, 305
master - mean (292ms) : 286, 297
section Bailout
This PR (8431) - mean (301ms) : 294, 309
master - mean (293ms) : 288, 297
section CallTarget+Inlining+NGEN
This PR (8431) - mean (1,176ms) : 1143, 1209
master - mean (1,167ms) : 1124, 1210
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8431) - mean (299ms) : 291, 308
master - mean (289ms) : 283, 296
section Bailout
This PR (8431) - mean (299ms) : 293, 305
master - mean (290ms) : 284, 296
section CallTarget+Inlining+NGEN
This PR (8431) - mean (1,088ms) : 985, 1191
master - mean (1,067ms) : 971, 1162
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-04-15 15:17:42 Comparing candidate commit adf6625 in PR branch Found 29 performance improvements and 36 performance regressions! Performance is the same for 214 metrics, 9 unstable metrics.
|
Contributor
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. 588 occurrences of : + db.user: postgres,
|
vandonr
commented
Apr 13, 2026
Comment on lines
+89
to
+96
| settings.AddScrubber(builder => | ||
| { | ||
| // scrub db.user tag because its presence varies | ||
| var lines = builder.ToString().Split(new[] { '\r', '\n' }, StringSplitOptions.None); | ||
| var filtered = string.Join(Environment.NewLine, lines.Where(line => !line.Contains("db.user:"))); | ||
| builder.Clear(); | ||
| builder.Append(filtered); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
I feel like there should be a better way to say "scrub this tag", but I didn't find it
Member
There was a problem hiding this comment.
um, it shouldn't vary should it? 🤔
Contributor
Author
There was a problem hiding this comment.
I think what happens is that when we run it locally it has a connection string with user sa, and when it runs in the CI, there is no user in the connection string. I can clarify the comment.
I can also try to see if we can unify this so that the behavior is the same locally or in the CI
Member
There was a problem hiding this comment.
And for the record, I believe you're looking for VerifyHelper.GetSpanVerifierSettings(), and the ScrubStringTags helper (wrap it, and pass your own implementation in that filters out the tag) 🙂
Contributor
Author
There was a problem hiding this comment.
I wrote a more explicit comment. I didn't find using a custom ScrubStringTags to be really cleaner because it's pretty verbose too, and requires passing all the arguments explicitly to GetSpanVerifierSettings
bouwkast
reviewed
Apr 15, 2026
| return new TagsCacheItem( | ||
| dbName: GetConnectionStringValue(builder, "Database", "Initial Catalog", "InitialCatalog"), | ||
| dbUser: GetConnectionStringValue(builder, "User ID", "UserID"), | ||
| dbUser: GetConnectionStringValue(builder, "User ID", "UserID", "User", "Uid", "Username", "User Name"), |
Collaborator
There was a problem hiding this comment.
I don't think we are the source of truth so I would say the confluence page is inaccurate. My understanding is that these need to be determined on a per-language per-integration basis
Member
There was a problem hiding this comment.
Yeah, the source of truth should really be here 😄 https://www.connectionstrings.com/
Contributor
Author
There was a problem hiding this comment.
in a sense, yes, but also, I don't think it's really an issue if we detect the user from a key that's not supposed to be in the connection string ? in what real world scenario would a custome complain that we detected the user from "User ID" on a DB where it's not the correct key to set the user ID ?
Contributor
Author
There was a problem hiding this comment.
I had forgotten about this website, I sent my robot buddy visit it, it came back with another missing key
Member
There was a problem hiding this comment.
I don't think it's really an issue if we detect the user from a key that's not supposed to be in the connection string
Come on, it's an easily avoidable bug 😝 What kind of shop do you think we're running here 😅
Contributor
Author
There was a problem hiding this comment.
I wouldn't say it's really a "bug", and I wouldn't say it's "easily avoidable" either: we don't have access to the DB type here for instance, and we don't check it for other fields (dbName and outHost). So, if it's something we really want to do I'm fine with it, but it's going to be a bigger code change, and add complexity to this for, I'd argue, very little benefit
Member
There was a problem hiding this comment.
Maybe I misunderstood the issue, then. Thanks.
vandonr
changed the title
bouwkast
approved these changes
Apr 15, 2026
vandonr
enabled auto-merge (squash)
April 16, 2026 15:06
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
a bunch of possible spellings were not included in the keys we look for when filling db.user including some we use in the integration tests connection strings, so I also edited the span metadata rules
Same for the host, but we didn't use those keys in tests
Reason for change
Implementation details
Test coverage
Other details