-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
#7105 Trace when query is done. This is needed in order to get Select… #7106
Conversation
Hi @fitzchak, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
@anpete I applied requested changes. |
public RelationalDataReader( | ||
[CanBeNull] IRelationalConnection connection, | ||
[NotNull] DbCommand command, | ||
[NotNull] DbDataReader reader) | ||
[NotNull] DbDataReader reader, | ||
[NotNull] DiagnosticSource diagnosticSource) |
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.
Add Check.NotNull
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.
Done
@@ -226,7 +226,7 @@ object IRelationalCommand.ExecuteScalar(IRelationalConnection connection, IReadO | |||
= new RelationalDataReader( | |||
connection, | |||
dbCommand, | |||
dbCommand.ExecuteReader()); | |||
dbCommand.ExecuteReader(), DiagnosticSource); |
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.
Move arg to newline.
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.
Done
@@ -65,6 +71,7 @@ public virtual void Dispose() | |||
{ | |||
if (!_disposed) | |||
{ | |||
_diagnosticSource.WriteDataReaderDisposed(this); |
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.
Looks like we are firing the event before disposing the underlying reader, which is probably more useful anyway. I think we should tweak the event name to reflect that. I.e. "Disposing" instead of "Disposed".
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.
Do you actually need the reader instance for your use case?
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.
Yes, we need it before the disposing. So I changed the name to "Disposing".
Also we need just the instance of DbDataReader
, which has the command with the statistics and the connection. So I changed this also to send just the DbDataReader.
I amended the commit with the requested changes. |
Fix #7105