Implementation of RecordException in TelemetrySpan - #1116
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1116 +/- ##
==========================================
+ Coverage 77.51% 77.67% +0.15%
==========================================
Files 222 222
Lines 6241 6272 +31
==========================================
+ Hits 4838 4872 +34
+ Misses 1403 1400 -3
|
| Dictionary<string, object> attributes = new Dictionary<string, object> | ||
| { | ||
| { SemanticConventions.AttributeExceptionType, ex.GetType().Name }, | ||
| { SemanticConventions.AttributeExceptionStacktrace, ex.ToInvariantString() }, |
There was a problem hiding this comment.
Due to the potential size of the stack trace, I feel that later we should have an option to not add it.
There was a problem hiding this comment.
it would be better to trim instead of not add?
There was a problem hiding this comment.
Do you mean an option in this method itself?
Or for individual exporters?
There was a problem hiding this comment.
Good questions.
It seems good to have "config" options to limit the size of attribute values in general: trim at some upper bound, and also an option to not add the stack trace. Both should be off by default: they are more like escape valves if this becomes a problem with actual usage.
I see these settings as part of API configuration. Anyway, I see it as separate from this PR.
CodeBlanch
left a comment
CodeBlanch
left a comment
There was a problem hiding this comment.
LGTM
@eddynaka Are you going to add an Activity extension that does similar? I'm thinking we could call it from instrumentation when an exception is detected?
something like this? public static void RecordException(this Activity activity, Exception ex)
{
// TODO....
}IF yes, what activityExtension should I add...we have some.... |
|
@eddynaka Ya that's what I was thinking. Maybe in here |
|
I will do that when I wake up :) |
Just pushed. Let me know what do you think! |
| { | ||
| // The set of constants matches the specification as of this commit. | ||
| // https://github.com/open-telemetry/opentelemetry-specification/tree/709293fe132709705f0e0dd4252992e87a6ec899/specification/trace/semantic_conventions | ||
| // https://github.com/open-telemetry/opentelemetry-specification/tree/master/specification/trace/semantic_conventions |
There was a problem hiding this comment.
nit: The comment above says this matches a particular commit. So lets link to that particular commit, not master as master changes.
5 participants
Fixes #1115.
Changes
Please provide a brief description of the changes here. Update the
CHANGELOG.mdfor non-trivial changes.For significant contributions please make sure you have completed the following items: