Implement metric identity specification #4222

jack-berg · 2022-02-28T18:56:32Z

Implements changes merged in spec PR #2317.

In summary:

Each instrument is identified by its name, description, unit, type, and value type.
If you ask a meter for an instrument with the same identity as one previously registered, you'll record to the same instrument.
If you ask a meter for an instrument the same name as one previously registered but some difference in description, unit, type, or value type, you've created an identity conflict. Multiple metrics with the same name will be exported, and we'll log a diagnostic error message.
Views also play a role in identity. Since you can use views to change the name, description, and aggregation, its fairly easy to create an identity conflict when you register multiple views that select the same instrument(s).

jack-berg · 2022-02-28T18:57:37Z

...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java

  public abstract String getDescription();

-  public abstract String getUnit();


No reason for getUnit() to be a top level field since views can't modify it.

jack-berg · 2022-02-28T18:58:10Z

...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java


-  /** The view that lead to the creation of this metric, if applicable. */
-  public abstract Optional<View> getSourceView();


No reason for getSourceView() to be optional. Getting rid of optional simplifies code.

jack-berg · 2022-02-28T19:00:19Z

sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/SdkMeterTest.java

-                    "Failed to register metric.")
-                .getThrowable())
-        .hasMessageContaining("Metric with same name and different descriptor already created.");
+    logs.assertContains("Found duplicate metric definition");


A lot of the tests in this code duplicate the test cases in IdentityTest, but with less complete validation. I think I should remove the duplicate scenarios to reduce maintenance burden.

codecov · 2022-02-28T19:15:01Z

Codecov Report

Merging #4222 (3e9e384) into main (26c0429) will decrease coverage by 0.11%.
The diff coverage is 91.98%.

@@             Coverage Diff              @@
##               main    #4222      +/-   ##
============================================
- Coverage     90.32%   90.21%   -0.11%     
- Complexity     4752     4762      +10     
============================================
  Files           553      555       +2     
  Lines         14611    14692      +81     
  Branches       1402     1413      +11     
============================================
+ Hits          13197    13254      +57     
- Misses          954      982      +28     
+ Partials        460      456       -4

Impacted Files	Coverage Δ
...lemetry/sdk/common/InstrumentationLibraryInfo.java	`100.00% <ø> (ø)`
.../java/io/opentelemetry/sdk/logs/SdkLogEmitter.java	`100.00% <ø> (ø)`
...n/java/io/opentelemetry/sdk/logs/data/LogData.java	`0.00% <0.00%> (ø)`
...va/io/opentelemetry/sdk/logs/data/LogDataImpl.java	`100.00% <ø> (ø)`
.../io/opentelemetry/sdk/metrics/data/MetricData.java	`85.00% <0.00%> (-4.48%)`	⬇️
...ry/sdk/metrics/internal/aggregator/Aggregator.java	`100.00% <ø> (ø)`
...dk/metrics/internal/aggregator/DropAggregator.java	`36.36% <ø> (ø)`
...k/metrics/internal/aggregator/EmptyMetricData.java	`100.00% <ø> (ø)`
...rics/internal/state/AsynchronousMetricStorage.java	`97.87% <ø> (ø)`
...nternal/state/DefaultSynchronousMetricStorage.java	`82.50% <ø> (ø)`
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59a6061...3e9e384. Read the comment docs.

anuraaga · 2022-03-01T07:15:22Z

...bledTest/java/io/opentelemetry/sdk/metrics/internal/descriptor/InstrumentDescriptorTest.java

+class InstrumentDescriptorTest {
+
+  @Test
+  void equals() {


Is it possible to use equalsverifier?

I can't figure out how to make it work with an abstract class implemented by autovalue.

anuraaga · 2022-03-01T07:22:03Z

...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java

  /** The instrument which lead to the creation of this metric. */
  public abstract InstrumentDescriptor getSourceInstrument();

+  /** The FQCN of the view aggregation. */


How about using Class directly instead of String?

Though otherwise if we already have the concept of a human-readable aggregation name for viewconfig, we should try to use that too

...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/descriptor/MetricDescriptor.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MeterSharedState.java

...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MetricStorageRegistry.java

…y-java into metric-identity

jack-berg · 2022-03-14T20:33:13Z

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/AggregationUtil.java

+          "Unrecognized aggregation " + aggregation.getClass().getName());
+    }
+    return name;
+  }


@anuraaga this is my solution for being able to translate aggregations to / from human readable names. Its in an internal class so we're not obligated to support it, but it provides a central place for the conversion that both the view file config and MetricDescriptor#getAggregationName() can depend on.

jack-berg · 2022-03-14T20:33:13Z

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/aggregator/AggregationUtil.java

+          "Unrecognized aggregation " + aggregation.getClass().getName());
+    }
+    return name;
+  }


@anuraaga this is my solution for being able to translate aggregations to / from human readable names. Its in an internal class so we're not obligated to support it, but it provides a central place for the conversion that both the view file config and MetricDescriptor#getAggregationName() can depend on.

Implement metric identity specification

f92ce7e

jack-berg requested review from anuraaga, jkwatson and jsuereth as code owners February 28, 2022 18:56

jack-berg requested a review from a user February 28, 2022 18:56

jack-berg requested a review from Oberon00 as a code owner February 28, 2022 18:56

jack-berg commented Feb 28, 2022

View reviewed changes

anuraaga reviewed Mar 1, 2022

View reviewed changes

jack-berg added 4 commits March 5, 2022 09:17

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

8d54990

…y-java into metric-identity

PR feedback

cd5b5bb

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

438e49d

…y-java into metric-identity

Fix build

88e3b4d

anuraaga mentioned this pull request Mar 10, 2022

Bring in micrometer shim #4258

Merged

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

3e9e384

…y-java into metric-identity

jack-berg commented Mar 14, 2022

View reviewed changes

anuraaga approved these changes Mar 15, 2022

View reviewed changes

jack-berg merged commit ccfa2dc into open-telemetry:main Mar 15, 2022

This was referenced Mar 16, 2022

Sample Groovy yields "Found duplicate metric definition" open-telemetry/opentelemetry-java-contrib#222

Closed

[spec compliance] Metrics SDK does not fail on duplicate name/different instrument metrics. #3809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement metric identity specification #4222

Implement metric identity specification #4222

jack-berg commented Feb 28, 2022

jack-berg Feb 28, 2022

jack-berg Feb 28, 2022

jack-berg Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

anuraaga Mar 1, 2022

jack-berg Mar 9, 2022

anuraaga Mar 1, 2022

anuraaga Mar 1, 2022

jack-berg Mar 14, 2022

jack-berg Mar 14, 2022

		public abstract String getDescription();

		public abstract String getUnit();


		/** The view that lead to the creation of this metric, if applicable. */
		public abstract Optional<View> getSourceView();

Implement metric identity specification #4222

Implement metric identity specification #4222

Conversation

jack-berg commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 28, 2022 •

edited