opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Tracing GA tracking

Open jtescher opened this issue 5 years ago • 11 comments

Spec compliance:

  • [x] Align Value with the spec (#317)
  • [x] ShouldSample's parent context parameter (#290)
  • [x] New trace config max defaults (#268)
  • [x] Ensure is_recording is false after span ends (#265)
  • [x] Add configuration options from environment variables (#168)
  • [x] Update the default propagator to be noop (#221)
  • [x] Update ExportResult: use std::result::Result type (#331)
  • [x] Decide on experimental non-specified propagators (#336)
  • [x] Consider moving exporters module to sdk (#340)
  • [x] Implement SpanProcessor::force_flush (#349)
  • [x] Don't send status unless ok or error in zipkin (#355)

API ergonomics:

  • [x] Allow &str and String as parameters where possible

Backwards compatibility:

  • [x] Ensure all pub items should be exposed
  • [x] Ensure dependencies in public APIs are 1.0
  • [x] consider dropping serde feature #576

jtescher avatar Nov 02 '20 19:11 jtescher

Ensure dependencies are 1.0

Feels like would be a difficult criteria to meet now as most of the crates in Rust are pre 1.0

TommyCpp avatar Dec 11 '20 03:12 TommyCpp

@TommyCpp good point, clarified it to only include ones in public interfaces

jtescher avatar Dec 11 '20 04:12 jtescher

The spec around tracing has stabilized and I see they are checking with language SIG on their progress to GA on tracing in the maintainer meeting logs. We might want to review the spec and see what we are missing. I can tell we still need to add http/json and http/protobuf support for opentelemetry-otlp.

Also, we might need to add logs in our API and SDK. Now that the opentelemetry log spec has decided that they will respect log facade from languages. I think we can just use Rust's log facade to instrument the logs for now and figure out if we want to use env_logger or our otlp log SDK later.

Rust also provides a guideline on library https://rust-lang.github.io/api-guidelines/checklist.html, we could walk through this and see if there are improvements we should make.

TommyCpp avatar Feb 25 '21 18:02 TommyCpp

I think we should probably use tracing instead of log?

djc avatar Feb 25 '21 19:02 djc

I think we should probably use tracing instead of log?

@djc I am a little worried that we would have cycle dependency if we want to use opentelemetry-log for log SDK and tracing for logging API because tracing-opentelemetry depends on opentelemetry

TommyCpp avatar Feb 27 '21 16:02 TommyCpp

Related to @TommyCpp - have you considered splitting out the sdk module in its own crate? It would be ideal to be able to isolate the instrumentation side (e.g. most of the traits) in a module moving faster towards 1.0 while keep iterating on the sdk side.

LukeMathWalker avatar Apr 07 '21 10:04 LukeMathWalker

have you considered splitting out the sdk module in its own crate?

That could work, but we need to first make sure the api doesn't use anything from SDK part otherwise there will be a circular dependency. Another problem is I believe there is a crate called opentelemetry-sdk already :disappointed:

On the other hand, we can still declare API stable while working on the SDK if I understand the VERSIONING.md correctly.

TommyCpp avatar Apr 08 '21 03:04 TommyCpp

If they are all part of the same crate and you break the public API of the sdk sub-module you are forced to publish a new major (if you are past 1.0) of the whole crate - api included. If you have two incompatible versions of opentelemetry around (e.g. 1.0 and 2.0) stuff like https://github.com/open-telemetry/opentelemetry-rust/blob/76b6ec9b7c5aba17a52b0e529d7026222a4e389a/opentelemetry/src/global/trace.rs#L237 stops working and it becomes very cumbersome for users of the crate to get traces propagation working correctly (i.e. everything that uses OpenTelemetry MUST be on the same version of opentelemetry).

I have just gone through this exciting exercise for 20+ applications and 10+ libraries (REST APIs, gRPC APIs, AMQP consumers, REST clients, etc.) - it takes a long time. That's why I am suggesting, if it reduces the chances of breaking changes in the past, to separate the api module from the sdk one - it would make it much easier for libraries in the Rust ecosystem to start bundling OpenTelemetry instrumentation (relying mostly on api) without having to do regular new releases.

LukeMathWalker avatar Apr 08 '21 08:04 LukeMathWalker

Yeah, you have a good point. I think we should review our status on backward compatibility and how far we are towards tracing GA. If we decided we still need a while to stable the SDK public API after we stabilized tracing API, maybe it's a good idea to separate the SDK from API. @jtescher / @djc / @frigus02 any thoughts?

TommyCpp avatar Apr 09 '21 03:04 TommyCpp

A lot of these are now done, could make sense to do another pass to see what changes are needed before GA, if the majority of the changes are in the SDK only then we may make life easier for some folks by separating them.

jtescher avatar Apr 09 '21 03:04 jtescher

Docs:

  • [ ] Add more context for how to choose runtime and why we need runtime #731

TommyCpp avatar Feb 15 '22 16:02 TommyCpp

Didn't realize this existed when I created https://github.com/open-telemetry/opentelemetry-rust/issues/977, but I have been keeping it updated there. So going to close this here. Please follow in #977

hdost avatar Feb 21 '24 07:02 hdost