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

Update activity status on failure MySqlDataReader. #1171

Merged
merged 2 commits into from
May 15, 2022
Merged

Update activity status on failure MySqlDataReader. #1171

merged 2 commits into from
May 15, 2022

Conversation

qq362220083
Copy link
Contributor

When MySqlDataReader has exception, the activity status needs to be updated.

Signed-off-by: qq362220083 <362220083@qq.com>
@qq362220083 qq362220083 changed the title Update activity status on failure execute. Update activity status on failure MySqlDataReader. May 13, 2022
public static void SetSuccess(this Activity activity) => activity.SetTag(StatusCodeTagName, "OK");
public static void SetSuccess(this Activity activity)
{
if (activity.Duration == TimeSpan.Zero)
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this change?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, is this a workaround for a lack of an IsStopped property (dotnet/runtime#63353) to check if the Activity is still running? A comment would be helpful here, but I think checking Status would be clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh, activity only supports Status property in .net 6 and above.

Here is the resolve to set the Exception first, and then set the Success to cause the state to be overwritten.

set the Exception should have the highest priority.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks; I hadn't checked the docs thoroughly to see where the API was available.

{
activity.SetException(ex);
activity.Stop();
}
dataReader.Dispose();
Copy link
Member

Choose a reason for hiding this comment

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

MySqlDataReader.DisposeAsync will still Stop the activity and set it to successful. Seems like that logic ought to change to if if (Activity.Status == ActivityStatusCode.Unset) to avoid overwriting the status if it's already an error.

@bgrainger
Copy link
Member

Thanks! This is definitely an omission in MySqlConnector's activity-handling code; thanks for fixing it. Left just a couple of comments about state management in the face of tracking this exception.

Signed-off-by: qq362220083 <362220083@qq.com>
@bgrainger bgrainger merged commit c9dfdd7 into mysql-net:master May 15, 2022
@bgrainger
Copy link
Member

Thanks for the contribution! This is included in 2.1.9.

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.

2 participants