Add record_exceptions options to with_span

[albertored]

Fixes #236

The Python API has been taken as a reference.

There is still something that does not fully convince me.

Elixir exceptions are recorded with exception.type equal to the name of the exception struct (see test).

Standard erlang errors are mapped by Elixir to exception structs so when in Elixir, inside a trace, an exception is raised with :erlang.error(type) the exception is recorded with the erlang type (e.g. error:badarg) but the user get an ArgumentError (see test).

[codecov]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
apps/opentelemetry/src/otel_tracer_default.erl	94.73% <90.00%> (-5.27%)	⬇️
apps/opentelemetry_api/src/otel_tracer.erl	69.56% <50.00%> (ø)
apps/opentelemetry_api/lib/open_telemetry/span.ex	22.72% <0.00%> (-2.28%)	⬇️
apps/opentelemetry_api/src/otel_span.erl	72.63% <78.78%> (-0.23%)	⬇️

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[tsloughter]

Hm, any danger in doing this instead of creating a new with_span in the Elixir macro instead of calling Erlang's with_span function?

I like to keep just one with_span function and hadn't considered we could do something like this to handle the exception case.

[albertored]

Replicating the whole with_span macro on Elixir side is the fallback option but first I wanted to explore this solution and I am quite satisfied with the result.

This piece of code fails if someone is doing

erlang:error(#{'__exception__' => true, '__struct__' => foo).

but it seems very unlikely. We can anyway protect from such cases and fallback to the normal type.

[albertored]

Also the call to Elixir.Exception.message() should be modified so that it doesn't fail in any circumstance

[tsloughter]

I was thinking about the relying on the internal structure of Elixir's structs and exceptions. Probably unlikely they change, but was my initial hesitation.

[albertored]

Ah ok, I think that structure is not internal Exception.t() is not an opaque type

[albertored]

But we may ask someone more involved with Elixir development, I don't know if there is some contributor here that can answer

[tsloughter]

Sorry for the delay. I'm not sure what to do about badarg vs ArgumentError either. Its probably best if it could be ArgumentError. Is that possible to do?

[albertored]

The only ways I can think of are:

• duplicating the implementation of the feature on Elixir side
• somehow add a metadata when using the Elixir version of with_span so that in elrang code we can handle this situation

I'm usually against code duplication but in this case I'm not sure the complexity needed for avoiding duplication is worth.

[tsloughter]

The problem with duplication is the logic is in the SDK and there is only the Erlang SDK. We'd have to have the logic in the API to add it to Elixir.

Hm, couldn't this just convert Erlang atoms like badarg to ArgumentError?

[albertored]

The problem is that in both these cases

 %% ERROR
    ?assertException(error, badarg, otel_tracer:with_span(Tracer, <<"span-error">>, #{record_exception => true},
                                               fun(_SpanCtx) ->
                                                erlang:error(badarg)
                                               end)),

    receive
        {span, SpanError} ->
            ?assertEqual(<<"span-error">>, SpanError#span.name),
            ?assertEqual(undefined, SpanError#span.status),
            [#event{name=exception, attributes=A}] = otel_events:list(SpanError#span.events),
            ?assertMatch(#{'exception.type' := <<"error:badarg">>, 'exception.stacktrace' := _}, otel_attributes:map(A))

    after
        1000 ->
            ct:fail(timeout)
    end,

and

assert_raise ArgumentError, fn ->
        Tracer.with_span "span-1", record_exception: true do
          :erlang.error(:badarg)
        end
      end

      assert_receive {:span,
                      span(
                        name: "span-1",
                        events: {:events, _, _, _, _, [event]},
                        status: :undefined
                      )}

      assert event(name: :exception, attributes: {:attributes, _, _, _, received_attirbutes}) =
               event

      assert %{
               "exception.type": "error:badarg",
               "exception.stacktrace": _
             } = received_attirbutes

the with_span function receives a error:badarg and it needs to know if it should be kept as is in exception.type or if it should convert it to ArgumentError

[tsloughter]

Yea, good point. I don't know what to do here. Maybe it has to stay as badarg.

@bryannaegele any thoughts or should we merge this?

[bryannaegele]

I don't know if it's 1:1 correlation to here, but Phoenix does normalization of erlang exceptions to something elixir can handle.

https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/main/instrumentation/opentelemetry_phoenix/lib/opentelemetry_phoenix/reason.ex

[albertored]

There is something similar also in Elixir core, where Erlang exception are translated to Elixir ones.

This is exactly what is causing problems here: the logic for recording the exception is in the SDK (Erlang side) and we need to know if the with_span is called from Erlang or Elixir in order to decide whether the exception should be translated to Elixir before being recorded.

The only way for doing so that I can think of is passing an argument to the with_span macro

[albertored]

I may have found a solution.

I take the first element of the stacktrace and use it to know if the exception was raised from Erlang or Elixir so that I can decide if the exception need to be translated or not.

In this way we also simplify the custom record_exception that was defined on Elixir API because know the SDK is able to handle it without customizations on top. A change on this is that know exception.type for Elixir exception is missing the Elixir. prefix, if it is a breaking change I can add it again

[bforchhammer]

Minor spelling mistake: /s/received_attirbutes/received_attributes