Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumentation scope - move to builder pattern? #1527

Open
cijothomas opened this issue Feb 13, 2024 · 11 comments · Fixed by #1567 · May be fixed by #2220
Open

Instrumentation scope - move to builder pattern? #1527

cijothomas opened this issue Feb 13, 2024 · 11 comments · Fixed by #1567 · May be fixed by #2220
Assignees
Labels
A-common Area:common issues that not related to specific pillar A-log Area: Issues related to logs A-metrics Area: issues related to metrics A-trace Area: issues related to tracing help wanted Good for taking. Extra help will be provided by maintainers/approvers

Comments

@cijothomas
Copy link
Member

#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 cijothomas added help wanted Good for taking. Extra help will be provided by maintainers/approvers A-metrics Area: issues related to metrics A-trace Area: issues related to tracing A-common Area:common issues that not related to specific pillar A-log Area: Issues related to logs labels Feb 13, 2024
@cijothomas
Copy link
Member Author

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

@hdost
Copy link
Contributor

hdost commented Feb 17, 2024

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

@izquierdo
Copy link
Contributor

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:

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
Copy link
Contributor

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
Copy link
Member Author

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

@cijothomas cijothomas reopened this Apr 26, 2024
@cijothomas
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Member Author

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
Copy link
Member

lalitb commented May 28, 2024

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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar A-log Area: Issues related to logs A-metrics Area: issues related to metrics A-trace Area: issues related to tracing help wanted Good for taking. Extra help will be provided by maintainers/approvers
Projects
None yet
6 participants