-
-
Notifications
You must be signed in to change notification settings - Fork 220
Fix SQLClient unplanned behaviors #1179
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1179 +/- ##
==========================================
+ Coverage 80.48% 80.51% +0.02%
==========================================
Files 212 212
Lines 6851 6887 +36
Branches 1565 1571 +6
==========================================
+ Hits 5514 5545 +31
Misses 823 823
- Partials 514 519 +5
Continue to review full report at Codecov.
|
src/Sentry.DiagnosticSource/Internals/DiagnosticSource/SentrySqlListener.cs
Outdated
Show resolved
Hide resolved
src/Sentry.DiagnosticSource/Internals/DiagnosticSource/SentrySqlListener.cs
Show resolved
Hide resolved
…sentry/sentry-dotnet into fix/sqlclient-firstspan-lost
Fix SpanTracer not initializing the Extra Dictionary. Add tests for SpanTracer SetExtra.
I had to initialize by default the Extra parameter from SpanTracer due to the chance of data loss when inserting data when the parameter wasn't created. |
if (connectionSpans?.Any(span => TryGetConnectionId(span) == connectionId) is false && | ||
connectionSpans.FirstOrDefault(span => TryGetOperationId(span) == operationId) is { } span) |
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.
Why iterate over connectionSpans twice inside this if?
this can have 1k items in it, looks highly inefficient with so many LINQ calls per execution
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.
The first iteration checks if there're any connection Spans with the given connection ID, if none is found it grabs the connection span with the given Operation ID and sets the connection ID.
I don't think you can perform both checks with only one iteration
This PR will fix the following issues found during dogfooding:
The first Query span is lost.
: It happens because oftentimes the event got invoked before the Connection event.
: FIX: create the Query on the Transaction if a connection is not found, afterwards, set the correct parent once the query is closed.
Missing
Connection Span
: The span was opened correctly but in case of data insertion or exclusion, the event related to closing the connection wasn't invoked, instead, another event gets invoked
WriteTransactionCommitAfter
.: FIX: Close the Connection span once
WriteTransactionCommitAfter
is invoked.Many
Connection Spans
with the sameconnection Id
:: Not sure why, perhaps since the connection might be shared, it closes to continue the execution of another query from another request and return to open the previous one once finished.
: FIX: Set the connection Id to the
Connection Span
if another Span with the sameConnection Id
isn't found.PS: Had to change the setter from the
SpanTrace.parentId
to internal in order to update theParentID
.