search

open-telemetry / opentelemetry-erlang

OpenTelemetry Erlang SDK
https://opentelemetry.io
Apache License 2.0
332 stars 105 forks source link

`attach` type specification out of date #443

Open philipcristiano opened 2 years ago

philipcristiano commented 2 years ago

attach takes a map() but that doesn't match the opentelemetry:span_ctx() type used elsewhere. This break Dialyzer when attempting to validate calls to attach

Can this be changed directly? I'm not sure the history for token in that file if the common type could be used instead.

tsloughter commented 2 years ago

Is a SpanCtx actually passed to it somewhere? That is wrong. It should be type t() not map().

philipcristiano commented 2 years ago

Is a SpanCtx actually passed to it somewhere?

Maybe not from this repository directly? I'm working from a comment to link traces from opentelemetry_cowboy.

The code appears to work, but is breaking Dialyzer.

aaronrenner commented 2 years ago

I am also seeing this issue when working with OpentelemetryProcessPropagator following this example.

https://github.com/open-telemetry/opentelemetry-erlang-contrib/blob/da912fd08b3bce7fd097c5f477e73db9e345fc3a/instrumentation/opentelemetry_ecto/lib/opentelemetry_ecto.ex#L111-L115

OpentelemetryProcessPropagator.fetch_parent_ctx/2 returns opentelemetry:span_ctx() | :undefined while OpenTelemetry.Ctx.attach/1 takes a map. I'm not sure which one is correct.

aaronrenner commented 2 years ago

The more I looked at it, it appears the types on OpentelemetryProcessPropagator need to be fixed for the case I mentioned above.

tsloughter commented 2 years ago

Yea, process propagator should be returning a Ctx not a SpanCtx when you are fetching the parent. So maybe that type spec is wrong?