-
Notifications
You must be signed in to change notification settings - Fork 104
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 to Elixir's Span module #138
Add RecordException to Elixir's Span module #138
Conversation
Codecov Report
@@ Coverage Diff @@
## main #138 +/- ##
===========================================
- Coverage 33.90% 14.03% -19.87%
===========================================
Files 64 30 -34
Lines 3256 463 -2793
===========================================
- Hits 1104 65 -1039
+ Misses 2152 398 -1754
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I would like some comments on how to approach this from Erlang side I researched about exceptions in Erlang, and they don't differ from actual values at all. It’s hard to settle on a good interface for I read here about how the |
I also increased the minimum Elixir version to |
Is "message" required or an optional field? The type in Erlang would be the "exception" (2nd part of I'd prefer "message" to be the result of a call to |
@tsloughter at least one of message or type should be present in the event. My whole issue with the Erlang side is about which arguments to accept. Should the function require that the user know what to pass? What if they mistakenly pass a incorrect argument? How would we know? |
It is on them to pass the right arguments, that is always true. But we can also optionally handle it for them in the Hm, this might be reason to replace the Elixir call of For Erlang I guess we need to offer a couple options, like The @ferd any thoughts on recording exceptions in Erlang? |
Based on what Python does https://github.com/open-telemetry/opentelemetry-python/blob/master/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L698-L700 maybe Need to check what Java uses for that field too. |
Feels fair to use the type as a class. The direct matched value is the reason (which is often a string in other languages and therefore it's alright to use tuples or strings there too for us), and stacktrace has direct matches as well. An interesting question will be to plan for stuff in EEP-54: erlang/otp#2849 -- this uses Args hidden in the stacktrace to add context to the exception's reason |
That PR introduces a Until then, would it be ok to use |
No, |
It also prints to stdout, it doesn't return a string. You just want |
@gugahoa are you able to update this to add the Erlang version? I'd like to get it merged before making a release soon. |
93aa6f0
to
38403db
Compare
8bddf4a
to
512f4a3
Compare
512f4a3
to
1f2893f
Compare
Reference for [`RecordException`](https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#record-exception) `RecordException` has been implemented only on Elixir side as it has a more uniform way to deal with exceptions as a value. The attribute `exception.escaped` is left up to the user to supply through the `attributes` parameter in `record_exception/4`.
8ff0a1f
to
6756f9f
Compare
Great, looking good. But now I'm not sure about requiring 1.11. At work we have systems still on 1.10 and 1.9 that aren't easy upgrades. Would at least 1.10 be not much work to support for this? |
The only thing that makes 1.11 required is the is_exception/1 guard, we can copy paste that into the module that uses it and remove the requirement on 1.11 Does that sound good to you? @tsloughter |
@gugahoa ah, yea, that sounds good. |
@tsloughter done 🔝 |
Stacktrace :: list(any()), | ||
Attributes :: opentelemetry:attributes(). | ||
record_exception(SpanCtx, Class, Term, Stacktrace, Attributes) -> | ||
ExceptionAttributes = [{<<"exception.type">>, iolist_to_binary(io_lib:format("~p:~p", [Class, Term], [{chars_limit, 50}]))}, |
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.
Instead of ~p
I think it should be ~0tP
, in which case a second argument of the term depth is needed. So like:
io_lib:format("~0tp:~0tP", [Class, Term, 10], [{chars_limit, 50}]))
The 0
means to not add newlines and the 10
means the depth of the term is limited to 10. I'm not sure if 10 is the best depth but seems reasonable.
What do you think @ferd ?
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.
Yeah a depth of 10 should be acceptable and it's easier to expand than it is to clamp down usually
Reference for
RecordException
RecordException
has been implemented only on Elixir side as it has amore uniform way to deal with exceptions as a value.
The attribute
exception.escaped
is left up to the user to supplythrough the
attributes
parameter inrecord_exception/4
.