Skip to content
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

Report telemetry with ActivitySource #1037

Merged
merged 4 commits into from
Oct 16, 2021
Merged

Report telemetry with ActivitySource #1037

merged 4 commits into from
Oct 16, 2021

Conversation

bgrainger
Copy link
Member

Fixes #1036.

Sample telemetry produced by connecting this PR to OpenTelemetry.Exporter.Jaeger.

image

Copy link

@vonzshik vonzshik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roji for some thoughts.

@@ -604,6 +606,9 @@ internal async Task DisposeAsync(IOBehavior ioBehavior, CancellationToken cancel
Command.CancellableCommand.SetTimeout(Constants.InfiniteTimeout);
connection.FinishQuerying(m_hasWarnings);

m_activity.SetSuccess();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another interesting thing: specification for database providers does not mention whether statement is completed whenever the first row is received, or every single row is read. SqlClient does the former.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noted that here: #1036:

TODO: Does the Execute activity end when ExecuteReader returns a DbDataReader, or when that DbDataReader has finished consuming the result set (and is closed/disposed)?

My preference is the latter; as an application developer, I usually care about when the entire using (var reader = cmd.ExecuteReader()) { ... } block has finished, not about when it "begins".

I suggested adding an event for when the DbDataReader is created and returned, for those who care about breaking the whole span into two parts.

It would be ideal to standardise this, have a good name for the event, etc.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See ongoing conversation in npgsql/npgsql#3967 (comment)

Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Signed-off-by: Bradley Grainger <bgrainger@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add ActivitySource
3 participants