Instrumentation scope - move to builder pattern?

[cijothomas]

#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). #1021 (comment) 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 #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