Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder #4711

jack-berg · 2022-08-22T17:57:16Z

Related to #4695.

…Builder

jack-berg · 2022-08-22T17:59:20Z

api/all/src/main/java/io/opentelemetry/api/metrics/MeterBuilder.java

+   * @return this
+   * @since 1.18.0
+   */
+  default MeterBuilder setAttributes(Attributes attributes) {


Can also make the argument the name should be setInstrumentationAttributes. The two setters currently don't align in this regard (setSchemaUrl and setInstrumentationVersion). I omitted "instrumentation" because it doesn't add any clarity - would have to be setScopeAttributes to improve clarify, which doesn't align with the existing naming pattern.

setStaticAttributes ? setFixedAttributes? If we want to stay with setAttributes (and I'm not opposed necessarily), we should add more clarifying javadoc about what they will be used for.

setCommonAttributes ?

The setters on these builders are all defining properties for scope, but scopes are an implementation detail that don't appear the SDK makes them available to exporters via SpanData, MetricData, LogData.

From the perspective of an API user, these properties just combine to define a unique tracer / meter / logger. I.e. if you specify a different name, version, schemaUrl, or attributes, you should expect to get a different instance.

I think setAttributes is probably most correct, but all of these javadoc could use clarification that distinct name, version, schemaUrl, attribute combinations return unique instances.

I've tried to improve the javadoc overall in LoggerBuilder, TracerBuilder, and MeterBuilder. Going to mark this as resolved but please reopen needed 🙂

sorry to reopen after all this time. I really don't like this method name, as-is. I would greatly prefer setScopeAttributes, but even that seems quite arcane.

In addition, the javadoc here doesn't explain at all what these attributes are for, or why one might want to assign them to a meter or tracer when they are created.

I hate to open up the bike-shedding, but we're going to have to explain this method to people if we don't make it crystal clear from the name + javadoc what this is for (honestly...not having followed the spec discussion about this, I don't know myself what the point of these attributes is).

api/all/src/main/java/io/opentelemetry/api/trace/TracerBuilder.java

github-actions · 2022-09-07T20:33:35Z

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

github-actions · 2022-09-21T22:33:21Z

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

mateuszrzeszutek · 2022-09-22T08:38:07Z

Now that open-telemetry/opentelemetry-specification#2789 was merged, should the attributes field be removed from InstrumentationScopeInfo equals()/hashCode() methods?

jack-berg · 2022-09-23T18:24:11Z

@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.

…y-java into builder-scope-attributes

jack-berg · 2022-09-23T19:22:50Z

sdk/common/src/main/java/io/opentelemetry/sdk/internal/ComponentRegistry.java

@@ -36,14 +38,16 @@ public ComponentRegistry(Function<InstrumentationScopeInfo, V> factory) {
   * any, otherwise creates a new instance and associates it with the given scope.
   */
  public V get(InstrumentationScopeInfo instrumentationScopeInfo) {
+    ScopeKey scopeKey = toScopeKey(instrumentationScopeInfo);


Note, the identity of Logger, Meter, Tracer is no longer strictly equal to InstrumentationScopeInfo identity because of this spec PR.

codecov · 2022-09-23T19:38:13Z

Codecov Report

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

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%)`	⬇️
...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%)`	⬇️
...telemetry/sdk/trace/export/BatchSpanProcessor.java	`93.10% <0.00%> (+2.06%)`	⬆️

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

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

github-actions · 2022-10-17T10:36:25Z

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

…y-java into builder-scope-attributes

api/all/src/main/java/io/opentelemetry/api/metrics/MeterBuilder.java

api/all/src/main/java/io/opentelemetry/api/trace/TracerBuilder.java

api/logs/src/main/java/io/opentelemetry/api/logs/LoggerBuilder.java

github-actions · 2022-11-03T04:37:59Z

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

…y-java into builder-scope-attributes

jack-berg · 2022-11-08T23:54:50Z

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

jkwatson

I think we need a better name for this method and better documentation about what this new use of attributes is for.

github-actions · 2022-11-23T00:40:50Z

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

jkwatson · 2022-11-23T04:53:06Z

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 · 2022-12-07T06:31:49Z

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

jack-berg · 2022-12-07T13:58:13Z

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 · 2022-12-07T14:31:57Z

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 · 2022-12-13T22:41:00Z

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.

Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitter…

16223ec

…Builder

jack-berg requested review from jkwatson and jsuereth as code owners August 22, 2022 17:57

jack-berg requested a review from a user August 22, 2022 17:57

jack-berg requested a review from Oberon00 as a code owner August 22, 2022 17:57

jack-berg commented Aug 22, 2022

View reviewed changes

jack-berg mentioned this pull request Aug 22, 2022

Add scope attributes #4695

Open

7 tasks

jkwatson reviewed Aug 24, 2022

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/trace/TracerBuilder.java Outdated Show resolved Hide resolved

github-actions bot added the Stale label Sep 7, 2022

jack-berg removed the Stale label Sep 7, 2022

github-actions bot added the Stale label Sep 21, 2022

jack-berg removed the Stale label Sep 21, 2022

jack-berg added 3 commits September 23, 2022 13:31

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

4e5f398

…y-java into builder-scope-attributes

Change Tracer, Meter, Logger identity to omit attributes

2deb1ea

Improve LoggerBuilder, MeterBuilder, TracerBuilder javadoc

07c1b55

jack-berg requested a review from a team September 23, 2022 19:20

jack-berg commented Sep 23, 2022

View reviewed changes

mateuszrzeszutek approved these changes Oct 3, 2022

View reviewed changes

github-actions bot added the Stale label Oct 17, 2022

mateuszrzeszutek removed the Stale label Oct 17, 2022

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

5902777

…y-java into builder-scope-attributes

trask approved these changes Oct 20, 2022

View reviewed changes

github-actions bot added the Stale label Nov 3, 2022

mateuszrzeszutek removed the Stale label Nov 3, 2022

jack-berg added 3 commits November 3, 2022 09:38

Fix typo in javadoc

d0cb834

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

6782bac

…y-java into builder-scope-attributes

Improve tests

0f64b36

jkwatson requested changes Nov 9, 2022

View reviewed changes

github-actions bot added the Stale label Nov 23, 2022

github-actions bot removed the Stale label Nov 23, 2022

github-actions bot added the Stale label Dec 7, 2022

github-actions bot removed the Stale label Dec 7, 2022

jack-berg closed this Dec 13, 2022

jack-berg mentioned this pull request Dec 16, 2022

Update LoggerBuilder, MeterBuilder, TracerBuilder javadoc #5050

Merged

jack-berg mentioned this pull request Jun 2, 2023

MeterBuilder must support attributes, as per spec #5503

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder #4711

Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder #4711

jack-berg commented Aug 22, 2022

jack-berg Aug 22, 2022

jkwatson Aug 24, 2022

jkwatson Aug 24, 2022

jack-berg Aug 24, 2022

jack-berg Sep 23, 2022

jkwatson Nov 9, 2022

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 21, 2022

mateuszrzeszutek commented Sep 22, 2022

jack-berg commented Sep 23, 2022

jack-berg Sep 23, 2022

codecov bot commented Sep 23, 2022 •

edited

Loading

github-actions bot commented Oct 17, 2022

github-actions bot commented Nov 3, 2022

jack-berg commented Nov 8, 2022

jkwatson left a comment

github-actions bot commented Nov 23, 2022

jkwatson commented Nov 23, 2022

github-actions bot commented Dec 7, 2022

jack-berg commented Dec 7, 2022

jkwatson commented Dec 7, 2022

jack-berg commented Dec 13, 2022

Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder #4711

Add scope attribute setter on MeterBuilder, TracerBuilder, LogEmitterBuilder #4711

Conversation

jack-berg commented Aug 22, 2022

jack-berg Aug 22, 2022

Choose a reason for hiding this comment

jkwatson Aug 24, 2022

Choose a reason for hiding this comment

jkwatson Aug 24, 2022

Choose a reason for hiding this comment

jack-berg Aug 24, 2022

Choose a reason for hiding this comment

jack-berg Sep 23, 2022

Choose a reason for hiding this comment

jkwatson Nov 9, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 7, 2022

github-actions bot commented Sep 21, 2022

mateuszrzeszutek commented Sep 22, 2022

jack-berg commented Sep 23, 2022

jack-berg Sep 23, 2022

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2022 • edited Loading

Codecov Report

github-actions bot commented Oct 17, 2022

github-actions bot commented Nov 3, 2022

jack-berg commented Nov 8, 2022

jkwatson left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 23, 2022

jkwatson commented Nov 23, 2022

github-actions bot commented Dec 7, 2022

jack-berg commented Dec 7, 2022

jkwatson commented Dec 7, 2022

jack-berg commented Dec 13, 2022

codecov bot commented Sep 23, 2022 •

edited

Loading