dnsimple / erldns

DNS server, in Erlang.
MIT License
398 stars 98 forks source link

Start opentelemetry_api otp application #142

Closed olof closed 1 year ago

olof commented 2 years ago

This is needed even in environments where opentelemetry itself isn't enabled, as it provides the interface for erldns to define its traces. Without this, erldns_worker would crash when processing queries, as the opentelemetry module doesn't exist:

{'module could not be loaded',
    [{opentelemetry,get_tracer,[erldns_worker],[]},
     {erldns_worker,handle_cast,2,
         [{file,".../erldns/src/erldns_worker.erl"},
          {line,69}]},

Note: I didn't notice this issue when starting my application using rebar3 shell; it was only when I deployed using a release (rebar3 release) this started to occur, so I assume it has to do with how the modules included in the release are collected. Without this change, relx wouldn't know to include the opentelemetry module (provided by the opentelemetry_api application), whereas rebar3 shell seems to have another another mechanism to find modules.

olof commented 2 years ago

(The test failure is addressed by #141.)

DXTimer commented 2 years ago

After reviewing the documentation and running some tests locally, I can confirm that the opentelemetry_api should be included as a direct dependency.

@olof if you can merge main into this branch we can approve and merge this PR.

Thanks for your contribution.

olof commented 2 years ago

@olof if you can merge main into this branch we can approve and merge this PR.

Updated (rebased, hope that's ok).

olof commented 2 years ago

New test failures; doesn't look directly related to my change from what can tell.

DXTimer commented 2 years ago

@olof the tests fail because now we have an actual reference to perform a type check and the currently used map() type for SpanCtx is invalid. The correct typing needs to be used in this case: ' otel_tracer:tracer_ctx()`. The type error only occurs in three places, would you be open to updating the type checks to get the specs to pass?

olof commented 2 years ago

Ah, got it! It was my fault after all :). Didn't realize the connection. I've just updated the pull request branch with your suggestion applied. Thanks!