Add support for schema URL

[jtescher]

See the spec change for details open-telemetry/opentelemetry-specification#1666

[TommyCpp]

We might want to discuss if it's possible to add a new configuration into tracer API without changing the public API. This feature was added after tracing spec stabled. So I imagine they could add more configuration into it.

I had a brief discussion with @carlosalberto in 1666 regarding that.

[dgetu]

Could I take this issue?

[jtescher]

@dgetu sure you can give it a shot 👍 keep in mind what @TommyCpp was mentioning about the public api, might be worth making a draft PR to see

[dgetu]

I'm not exactly sure where to start with that. I've only just started getting involved with the project.

[TommyCpp]

I guess the idea is to design the API so that we can add other configurations in the future without breaking the existing code. Other languages support it with optional parameters. Need to figure out what's the best choice in Rust.

[dgetu]

I don't think I have enough experience with either the language or the codebase to provide anything meaningful to that discussion. I'd still like to contribute however I can.

[dgetu]

I took another look at spec 1666, and I think I'm starting to see the problem. The only way I can think of to implement this without it being a breaking change is to implement new function headers. Either a replacement for an overload a la Java or a setter for a builder pattern. Is that the kind of change to the tracer API you wanted to see made to the spec? I guess I'm not sure what the difference is between "tracer API" and "public API."

[TommyCpp]

We haven't declared GA yet so it's fine to make breaking change as for now. I want to explore if we can rebuild the Rust tracer API so that in the future we can add more options to it if needed. The current implementation doesn't support that because if we add another parameter to the function, it will break existing code.

I feel like we can do it either by having a builder for the tracer or having a configuration object as the parameter(like what Golang did). So I don't think we need to change the spec API.

[dgetu]

I think that having a config struct would be more clear than a builder. I can try both and see which one is more suitable.

[dgetu]

I'm in the process of migrating to a single config struct parameter for TracerProvider::get_tracer, but I'm not sure how to go about building it. My current approach is to have the user update their own config struct using Default, but this poses a problem. When every field in the struct is populated, attempting to update it results in a linter warning, which I have reservations about suppressing.

The larger issue is that the caller must remember to update the struct, even when populating every field. If the caller forgets, their code would break the next time the struct has a field added. One solution I'm aware of would be to use a builder for the config struct, but I might as well use a builder for TracerProvider directly in that case. The only other is to declare the struct as non_exhaustive, but callers outside of the crate would need a builder to create the struct. Again, I might as well use a builder for TracerProvider directly in that case.

If anyone with more experience with Rust could point me in the right direction towards making this work (ideally more idiomatically), I'd appreciate that. Otherwise, I'll go ahead and move forward with refactoring TracerProvider with the builder pattern.

[TommyCpp]

@dgetu As we are considering the possibility to add more fields into the TracerConfig. It may be reasonable to mark it with #non_exhaustive attribute. We should probably provide with_<attribute> method on TracerConfig so that users can change the configuration as they need. Similar to what we did on tracing sdk config.

Note that the config we pass into the get_tracer function is used to config each tracer. One tracer provider could have different configurations on different tracers. So the configuration struct is more like TracerConfig rather than TracerProviderConfig.

[TommyCpp]

May also need to add schema URL support for Resouce. See open-telemetry/opentelemetry-specification#1692

[TommyCpp]

Have thought about this a little bit. I wonder if we could provide a config struct and a macro to have better ergonomics.
Could have something like

let tracer = Tracer::new(TracerConfig{
name: "test",
..Default::default()
})

equal to

let tracer = tracer!("test");

I think macro could support the different number of parameters.

[dgetu]

Thanks for the suggestion! I think a macro would be nice to have. I'll see what I can do.

[jtescher]

Would a second method be sufficient here? wondering if adding something like TracerProvider::tracer_with_schema that takes a name, optional version, and schema would be the simplest.

[jtescher]

Could be good to experiment with macros as well as the spec-defined API is not the most ergonomic, but might be easiest to experiment with alternate APIs in a new package if we want to get a v1 of tracing out in the near future

[TommyCpp]

It's kind of wired that trace_with_version doesn't include schema parameter but tracer_with_schema takes a version parameter. They are both optional parameters as per spec.

I guess the real problem here what should we do if the spec added a new parameter for tracer or other API after we reaches v1.

[jtescher]

@TommyCpp yeah with 3 parameters 2 of which are optional the API becomes a little messy. Another option would be to have a "simple" version like TracerProvider::tracer that just takes a name, and then something like a TracerProvider::tracer_builder method for more complex tracers similar to the Tracer::start vs Tracer::span_builder split.

[realtimetodie]

What is the current state? I see @dgetu was assigned