-
Notifications
You must be signed in to change notification settings - Fork 5k
Record exception in HttpClient native instrumentation #115959
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl |
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 good as-is, thank you!
Failures in CI look unrelated. |
if (exception is not null) | ||
{ | ||
// Records the exception as per https://github.com/open-telemetry/opentelemetry-specification/blob/v1.45.0/specification/trace/exceptions.md | ||
activity.AddException(exception); |
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.
Actually one thought: This call will record the exception event with a timestamp slightly after the Activity
end time. I'm not sure if this will cause any trouble on the monitoring side, but we could avoid the issue by passing timestamp: activity.StartTimeUtc + activity.Duration
here.
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.
I am not sure to understand this point. How the exception event can have a timestamp after the activity end timestamp?
The activity is stopped later l221:
activity.Stop(); |
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.
There is a SetEndTime()
call at the top of the finally
block:
activity.SetEndTime(DateTime.UtcNow); |
activity.Stop()
will not update value of Duration
value if it has been set manually earlier by calling SetEndTime()
. (Note that there is no EndTime property SetEndTime()
assigns the Duration
.)
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.
Good catch! I missed that. I applied the suggestion and added a comment. Please let me know if it's fine for you.
Fixes #108050.