-
Notifications
You must be signed in to change notification settings - Fork 773
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
Set Activity.Status when adding SetErrorStatusOnException #4336
Set Activity.Status when adding SetErrorStatusOnException #4336
Conversation
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.
Not clear how the linked issue is addressed with this PR. The linked issue can be fixed by setting status on Activity (w/o using tags)
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.
Please add changelog
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
=======================================
Coverage 84.72% 84.72%
=======================================
Files 298 298
Lines 11983 11984 +1
=======================================
+ Hits 10152 10153 +1
Misses 1831 1831
|
@@ -75,7 +75,11 @@ public override void OnEnd(Activity activity) | |||
|
|||
if (snapshot != pointers) | |||
{ | |||
// TODO: Remove this when SetStatus is deprecated | |||
activity.SetStatus(Status.Error); |
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 assume there's never a time we'd call this version of SetStatus
and not also want to call the "real" version, right?
Here's another spot
span.SetStatus(Status.Error); |
We could kill two stones with one bird if we moved the call to activity.SetStatus(ActivityStatusCode)
to our extension method. Though, from what I can tell I think that OpenTracing shim is the only other place... so maybe easier to just duplicate the call there as well.
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.
@alanwest I was not aware of this one, will get it updated. what do you think about marking our extension method obsolete?
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 would support marking it obsolete.
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.
+1
Fixes #4333
Design discussion issue #
Changes
Please provide a brief description of the changes here.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes