-
Notifications
You must be signed in to change notification settings - Fork 752
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
Add RecordException overload accepting additional tags #3433
Add RecordException overload accepting additional tags #3433
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3433 +/- ##
==========================================
+ Coverage 86.21% 86.39% +0.18%
==========================================
Files 265 265
Lines 9598 9602 +4
==========================================
+ Hits 8275 8296 +21
+ Misses 1323 1306 -17
|
@alanwest Could we use TagList? Reasons...
If you do decide to go with public static void RecordException(this Activity activity, Exception ex, in TagList tags) |
I agree with the benefits, but I guess the reason I didn't reach for TagList is that it is not used by any of the existing Activity APIs (might fact check me on this...). I was inclined to consider Also, one might argue that
|
The spec is not marked stable for the Exceptions part.! |
IIRC, TagList was added while adding Metrics. It was intentionally not placed in .Metrics namespace, to re-use it in the future for Tracing as well. Its something we need to ask to be supported by |
Ya like @cijothomas said I think that is just because it came later 😄
I think someday it will get there 😄 dotnet/runtime#53641
You mean have the method accept the interface and let user pass in If you made it |
and
Understood - though wasn't aware of the already open issue. The thing I was unsure of was whether once it was part of the Activity API what type would be used. Sounds like it's expected that |
@@ -92,6 +104,11 @@ public static void RecordException(this Activity activity, Exception ex) | |||
tagsCollection.Add(SemanticConventions.AttributeExceptionMessage, ex.Message); | |||
} | |||
|
|||
foreach (var tag in 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.
Add a null
check for activity
in the beginning to return
if the activity
is null
and avoid creating tagsCollection
and copying from tagList
.
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.
Added null check.
This brought my attention to a somewhat interesting question: what if ex
is null, but tags
is not?
One might argue that the only reason someone would call RecordException
is if they had a non-null exception and that if they just wanted to add an event then they'd call Activity.AddEvent
.
But what do folks think? Should we add an event anyways even though no exception related attributes may end up on the event?
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.
Should we add an event anyways even though no exception related attributes may end up on the event?
It doesn't sound right to me. I assume most people would use this in the catch
block or some callback for when an exception occurs. So, it should not be a common scenario. I would say, as you mentioned, they should just use Activity.AddEvent
if it has nothing to do with an actual 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.
Left a suggestion. Looks good overall.
…elemetry-dotnet into alanwest/record-exception
…elemetry-dotnet into alanwest/record-exception
/// <param name="ex">Exception to be recorded.</param> | ||
/// <param name="tags">Additional tags to record on the event.</param> | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
public static void RecordException(this Activity activity, Exception ex, in TagList 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.
unless Activity.AddEvent/team also accepts TagList, the benefits of using TagList in this API is nil, as we anyway have to allocate the ActivityTagsCollection.
Hopefully if/when the spec is stable, we can bring this up.
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 think the idea is that the consumer can still avoid some allocations when using TagList
, but yea the benefit would be greater if AddEvent
accepted TagList 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.
Please update the PR description with the final shape of the API.
Fixes #3369.
Adds
RecordException
overload accepting additional tags to add to the exception event.