Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder

[jack-berg]

Related to #4695.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mateuszrzeszutek]

Now that https://github.com/open-telemetry/opentelemetry-specification/pull/2789 was merged, should the attributes field be removed from InstrumentationScopeInfo equals()/hashCode() methods?

[jack-berg]

@mateuszrzeszutek addressing that has been on my todo list! It minimally needs to be reflected in the ComponentRegistry's key definition of what constitutes a distinct instrumentation scope, which currently uses InstrumentationScopeInfo equals()/hashCode().

However, I think we can keep InstrumentationScopeInfo equals()/hashCode() as is. InstrumentationScopeInfo is a record class, and although spec API / SDK defines a definition of of tracer / meter / logger uniqueness which excludes scope attributes, that feels like an internal implementation detail which doesn't need to bubble up to the public API. It's still useful to have a equals / hashcode definition which reflects equality in the traditional java sense, where all fields are considered.

[codecov[bot]]

Codecov Report

Base: 90.92% // Head: 90.93% // Increases project coverage by +0.00% :tada:

Coverage data is based on head (0f64b36) compared to base (f8a4d81). Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #4711   +/-   ##
=========================================
  Coverage     90.92%   90.93%           
- Complexity     4816     4821    +5     
=========================================
  Files           545      547    +2     
  Lines         14382    14391    +9     
  Branches       1382     1382           
=========================================
+ Hits          13077    13086    +9     
- Misses          898      899    +1     
+ Partials        407      406    -1     

Impacted Files	Coverage Δ
.../opentelemetry/api/logs/DefaultLoggerProvider.java	84.61% <0.00%> (-7.06%)	:arrow_down:
...ava/io/opentelemetry/api/metrics/MeterBuilder.java	100.00% <100.00%> (ø)
...java/io/opentelemetry/api/trace/TracerBuilder.java	100.00% <100.00%> (ø)
...va/io/opentelemetry/sdk/logs/SdkLoggerBuilder.java	100.00% <100.00%> (ø)
.../io/opentelemetry/sdk/metrics/SdkMeterBuilder.java	100.00% <100.00%> (ø)
...a/io/opentelemetry/sdk/trace/SdkTracerBuilder.java	100.00% <100.00%> (ø)
...metry/sdk/metrics/export/PeriodicMetricReader.java	87.14% <0.00%> (-2.86%)	:arrow_down:
...telemetry/sdk/trace/export/BatchSpanProcessor.java	93.10% <0.00%> (+2.06%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jack-berg]

@jkwatson can you take a look when you have a chance?

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jkwatson]

We had discussed during the SIG meeting that perhaps what we need is a scope object we can set on the builder, rather than having to do it piecemeal.

[github-actions[bot]]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[jack-berg]

We had discussed during the SIG meeting that perhaps what we need is a scope object we can set on the builder, rather than having to do it piecemeal.

@jkwatson I think we should close this until users ask for it. Adding a scope object is nice from an API ergonomics perspective, but means that we'll have a 3rd class floating around representing a scope-like thing: InstrumentationScopeInfo, the deprecated InstrumentationLibraryInfo, and whatever we call this new scope. InstrumentationScopeInfo would do the trick but we can't move InstrumentationScopeInfo to the API module without a breaking change.

[jkwatson]

We had discussed during the SIG meeting that perhaps what we need is a scope object we can set on the builder, rather than having to do it piecemeal.

@jkwatson I think we should close this until users ask for it. Adding a scope object is nice from an API ergonomics perspective, but means that we'll have a 3rd class floating around representing a scope-like thing: InstrumentationScopeInfo, the deprecated InstrumentationLibraryInfo, and whatever we call this new scope. InstrumentationScopeInfo would do the trick but we can't move InstrumentationScopeInfo to the API module without a breaking change.

Sounds fine to me. Just thinking out loud here, but could we create a Scope interface in the API that is additionally implemented by InstrumentationScopeInfo? Or would that not add much value?

[jack-berg]

Sounds fine to me. Just thinking out loud here, but could we create a Scope interface in the API that is additionally implemented by InstrumentationScopeInfo? Or would that not add much value?

I don't think it would add much value because the consumers of these are totally different. The caller of the API is an instrumentation author, and the consumer of scope in the SDK is probably someone writing an exporter. Would want something to keep the two implementations in sync with each other, but hard for it to not feel awkward.