tokio-rs / tracing-opentelemetry

MIT License
228 stars 77 forks source link

Outdated docs: `SpanKind` doesn't implement `Display` anymore #151

Open survived opened 3 months ago

survived commented 3 months ago

Bug Report

The crate docs suggest setting span kind as "otel.kind" = %SpanKind::Client, however, it doesn't compile as SpanKind doesn't implement Display since opentelemetry v0.18 (you can see that display is implemented in v0.17, but in v0.18 it's gone).

https://github.com/tokio-rs/tracing-opentelemetry/blob/9462de6481321704912f54d3f61c080eb732ca62/src/lib.rs#L34-L37

Version

v0.24.0, rendered on docs.rs

djc commented 3 months ago

Thanks for the report. Want to send a PR to change the % to ??

survived commented 3 months ago

I don't mind sending PR, but I'm not sure that changing % with ? will actually work. Technically, we shouldn't make any assumptions on what std::fmt::Debug implementation outputs, it's supposed to be used in debugging context. In this particular case, Debug implementation will output strings with the first letter capitalized. E.g. format!("{:?}, SpanKind::Client) outputs "Client", whereas Display implementation used to output "client" (all letters are lower-case), which I assume is expected string-representation of the SpanKind.

I don't know what's the best way to address that. The easiest way is to update doc so it directly uses strings: "otel.kind" = %"client" (and maybe refers to the spec where all possible values and their string-representations are listed). The other option is to expose a module like pub mod span_kind { static CLIENT: &str = "client"; /* ... */ }.

djc commented 3 months ago

This was removed in https://github.com/open-telemetry/opentelemetry-rust/pull/758, which asserts the formatting is Jaeger-specific. Might be best to open an issue upstream to discuss further?

survived commented 3 months ago

I don't know what's the best way to proceed tbh since I'm not familiar with otel at all. If the formatting is indeed not a part of the spec, it makes sense to me to replace the SpanKind with the string, i.e. write "otel.kind" = %"client"

djc commented 3 months ago

I don't see an otel.kind attribute here:

https://opentelemetry.io/docs/specs/semconv/attributes-registry/otel/

SpanKind is documented here:

https://opentelemetry.io/docs/specs/otel/trace/api/#spankind

But without an explicit way to Display. So I guess we should maybe remove this attribute from the docs?

survived commented 3 months ago

So I guess we should maybe remove this attribute from the docs?

It does make sense to me!

survived commented 3 months ago

However, looking at the code, it might be not as trivial. Span kind is used here:

https://github.com/tokio-rs/tracing-opentelemetry/blob/7f4409a935608bb89e970956217a91945f285b0f/src/tracer.rs#L82-L89

if we remove it, should_sample will always be called with SpanKind::Internal, that doesn't sound good

djc commented 3 months ago

I was only talking about removing it from the docs, which I don't think would affect that usage?

mladedav commented 3 months ago

I think the example should be changed to otel.kind = "client" so that it at least compiles.

It's a bit unfortunate that this is the example provided as span kind is not a semantic convention, it is part of the span itself. This is just the way tracing-opentelemetry lets the user set it and it should probably be documented somewhere else.

djc commented 3 months ago

I think the example should be changed to otel.kind = "client" so that it at least compiles.

It's a bit unfortunate that this is the example provided as span kind is not a semantic convention, it is part of the span itself. This is just the way tracing-opentelemetry lets the user set it and it should probably be documented somewhere else.

Sounds like you have informed opinions, would you be able to submit a PR for this? 👍