Instrumentation scope - move to builder pattern?

[cijothomas]

trafficstars

https://github.com/open-telemetry/opentelemetry-rust/pull/1021 added supported for Attributes to instrumentation scope, which already had version, schema_url. This started with just name and evolved to add more and more (because spec added them). https://github.com/open-telemetry/opentelemetry-rust/pull/1021#discussion_r1161364638 called out the need for potentially making this to a builder pattern, so as to protect us from breaking change in the future, if there are more stuff added to scope.

let tracer = TracerProvider.tracer("tracer_name"); // common case. this is still a good idea to keep to keep things simple for majority.
let tracer = TracerProvider::tracer_builder("tracer_name") // advanced scenario.
    .with_version("1.0.0")
    .with_attributes([kv])
    .build();

Opening an issue to explore if moving to builder pattern makes sense here (all 3 signals), and if yes, implement the actual change. Also, there could be other places where such changes are required to make things cleaner/more idiomatic and also protect from future breaking changes.

[cijothomas]

Have reached out to @izquierdo to look into this. Thanks in advance!

[hdost]

In general we should try to have builder pattern for object creation since variatic arguments aren't available.

[izquierdo]

I would like some design input regarding the builders.

Since the builder requires a TracerProvider when the time comes to build the Tracer, I tried two different patterns, both of which are currently available in https://github.com/open-telemetry/opentelemetry-rust/pull/1567:

• Clone the TracerProvider into the builder
  • Implemented as TracerBuilder1 in the PR
  • Usage: https://github.com/open-telemetry/opentelemetry-rust/pull/1567/files#diff-01ef317f6fa48e1fe2b6ce88329d7769a694190abbc899e6b2076ce9a8197edfR107-R114
  • Downside: requires TracerProvider to be cloned
• Require passing the TracerProvider to the builder at build time
  • Implemented as TracerBuilder2 in the PR
  • Usage; https://github.com/open-telemetry/opentelemetry-rust/pull/1567/files#diff-01ef317f6fa48e1fe2b6ce88329d7769a694190abbc899e6b2076ce9a8197edfR221-R229
  • Downside: the builder is not self-contained

Any opinions on which approach is best? Or maybe another approach I haven't considered? I'm leaning towards the TracerBuilder1 way because it seems friendlier to users and avoids the need for users to directly create the builder from outside of TracerProvider.

Once a path has been decided I will clean up that PR by:

• Renaming the selected builder to TracerBuilder and removing the other one
• Adding missing documentation and fixing any failed checks
• Dealing with logs and metrics in a similar way

[stormshield-fabs]

It think the first option is better, especially considering the "self-contained" concern you raised. We could have it take a reference to the TracerProvider, avoiding the Clone problem.

#[derive(Debug)]
pub struct TracerBuilder1<'a, T: TracerProvider> {
    provider: &'a T,
    library_builder: InstrumentationLibraryBuilder,
}

Also, we should maybe only expose TracerProvider::tracer_builder and remove TracerBuilder::new.

[cijothomas]

Reopening as we still need to do the same for Metrics too. Logs/Traces are completed via #1567

[cijothomas]

@stormshield-fabs has agreed to implement this for Metrics. This is a breaking change to the API, so would prefer to do as much of them before beta!

[TommyCpp]

Proposed changes in 5/21 community calls

TracerProvider::tracer_builder("tracer_name") // advanced scenario.
    .with_instrutation_library(InstrumentationLibrary::default().version(XXX).schema_url(xxxx))
    .build();

[cijothomas]

Proposed changes in 5/21 community calls

TracerProvider::tracer_builder("tracer_name") // advanced scenario.
    .with_instrutation_library(InstrumentationLibrary::default().version(XXX).schema_url(xxxx))
    .build();

Not sure why InstrumentationLibrary is part of public API? The SDK will make an instrumentation library based on tracer name/ver/etc., but not able to find why that is part opentelemetry public API!

The following is the current API for tracer...

let tracer = global::tracer_provider().tracer_builder("my-library-name").
         with_version(env!("CARGO_PKG_VERSION")).
         with_schema_url("https://opentelemetry.io/schemas/1.17.0").
         build();

I'll watch the recording from the last part of the call I missed and get back.

[TommyCpp]

@stormshield-fabs 's concern was we will repeat the with_version and with_schema_url functions across all signals.

In generally I think we need some kind of "namespace" in builder patterns. If one builder takes all information for the field of their field then I worry the builder functions number will be too large to quickly find the configuration needed.

Where do we draw the line in terms of which info should stay as a separate struct with their own builder/constructor is the question

[cijothomas]

5/28 SIG meeting: Check if there is any history behind InstrumentationLibrary being a pub struct. Potentially hide it from doc. Continue with builder patter for Metrics, just like logs, traces, potentially cleaning up logs/traces to remove InstrumentationLibrary usage in public API.

[lalitb]

Added this issue in Logs beta release milestone, as it involves deprecating/removing below method from Logs Bridge API surface:

rust fn library_logger(&self, library: Arc<InstrumentationLibrary>) -> Self::Logger;