Migrate Metrics SDK "data' package to Attributes #3352

jsuereth · 2021-06-26T18:23:33Z

With the Labels removal in the new Metrics API, this is a first cut at changing the existing implementation in preparation for the new SDK.

What this PR brings:

The io.opentelmetry.sdk.metrics.data package now uses the io.opentelemetry.api.common.Attributes type instead of the old Labels type.
An AssertJ testing DSL for all the various types in the *.metrics.data package. This is designed to:
- Solve floating-point equality precision-in-testing concerns in one locations
- Be resilient to order-concerns of points / attributes
- Have easily readable expectations and error messages
Update all existing tests that either used Labels or relied on ordering/equality of Labels instances to use new Assertj library for metrics.

Note: Summary is effectively deprecated in the OTel Data model (for backwards compat primarily), so it remains for OpenCensus Shims (Summary is also deprecated in OpenCensus) and I did not provide convenience APIs for summary. In the new SDK no instrument will output a summary by default.

…rary.

- Finish wiring through changes to all code/tests. - DOES NOT touch API - Update existing tests to use new testing library to be sensitive to attribute vs. label hashing alterations + equality of the metrics.data package types.

anuraaga

Just some small comments, no blockers. Thanks!

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java

anuraaga · 2021-06-28T04:38:16Z

exporters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/MetricAdapter.java

+              labelNames.add(sanitizedLabelName);
+              // TODO: We want to create an error-log if there is overlap in toString of attribute
+              // values for the same key name.
+              labelValues.add(value == null ? "" : value.toString());


Don't believe we support null values anymore, think we may have at some point before

should I remove the check or keep it for safety?

anuraaga · 2021-06-28T04:40:04Z

opencensus-shim/src/test/java/io/opentelemetry/opencensusshim/InteroperabilityTest.java

-    assertThat(pointData.getLabels().size()).isEqualTo(0);
-
+    assertThat(metric)
+        .hasName("double_gauge")


anuraaga · 2021-06-28T04:41:16Z

opencensus-shim/src/test/java/io/opentelemetry/opencensusshim/InteroperabilityTest.java

+        .hasDescription("double gauge")
+        .hasUnit("ms")
+        .hasDoubleGauge()
+        .points()


Tend to like not switching gears in an assertion (i.e., allowing a further assertion on the metric after this one) with something like pointsSatisfyExactly(...). One reason is for the indentation to match the structure. I guess it's mostly just personal preference though so don't mind too much

I hear you. I chose the methods to match existing patterns in Assertj, particularly around Iterables/Collections.

The TL;DR: is this matches Assertj even if I'm more of a hamcrest fan myself (primarily for composability).

Want to call out two things:

The goal of this library is folks STOP using exact ordering where it's not important (you can choose when, but prefer inAnyOrder if you don't know whether order matters). If you look at examples there's a lot of .points().satisfiesExactlyInAnyOrder.

There's a lot of Iterable matches I don't want to do a combinatorial explosion on providing, similar for Attributes. I could take an approach here (similar to what I did for Attribtues) where you can go into a .points or use a hasPoints that implicitly uses "containsInAnyOrder".

anuraaga · 2021-06-28T04:42:58Z

sdk/metrics-testing/build.gradle.kts

+}
+
+description = "OpenTelemetry SDK Testing utilities"
+otelJava.moduleName.set("io.opentelemetry.sdk.metrics-testing")


We've generally suffixed with metrics in all our packages (even had a gradle hack that matched the suffix before the more declarative gradle.properties :P). Since alpha and eventually will be merged into testing doesn't matter too much though

If possible, I'd like to avoid chaning the module name (and directory) because I have a slew of PRs lined up behind this one for the new metrics implementation and the least amount of major rebasing I need to do, the better.

If this were a fundamental naming issue (or something that wouldn't be resolved when metrics goes stable) I'd be happy to change it, but as things are my commits are starting to get unwieldy if I leave this two long (hence trying to piece out components like this one for review).

we can always go back and change module names at the end of the PR chain, if anyone feels strongly about it.

anuraaga · 2021-06-28T04:46:17Z

...ics-testing/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/MetricDataAssert.java

+  }
+
+  /** Ensures the {@link Resource} associated with a metric matches the expected value. */
+  public MetricDataAssert hasResource(Resource resource) {


What do you think about just hasResourceAttributes and hasResourceAttributesSatisfying? Resource only has one field, attributes

While I half-agree, in practice almost all the tests (and code) just pass Resource wholesale, and check for the one static instance that was given to an SDK. Very rarely are folks checking resource detection/attribute scenarios in Metrics code.

I imagine you could add those two methods, but I don't think I'd wind up using them for most of the metrics tests.

just FYI, Resource also has schemaUrl now; not sure if that alters the fundamentals, though.

sdk/metrics/build.gradle.kts

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/aggregator/MetricDataUtils.java

pirgeo

There seems to be an empty file named point.getAttributes()) in the repo root. Is this intentional?

jsuereth · 2021-06-28T14:26:26Z

There seems to be an empty file named point.getAttributes()) in the repo root. Is this intentional?

@pirgeo No, that was a botched commit from windows on my part. Must have done something dumb w/ copy-paste on the terminal. Fixed.

jkwatson · 2021-06-28T21:02:49Z

...ting/src/main/java/io/opentelemetry/sdk/testing/assertj/metrics/AbstractPointDataAssert.java

+
+/** Test assertions for {@link PointData}. */
+public class AbstractPointDataAssert<
+        PointAssertT extends AbstractPointDataAssert<PointAssertT, PointT>,


this style of type declaration isn't something we've used in the project up until now. I'm not fundamentally opposed to it, but it's definitely more verbose. @anuraaga any thoughts on this? Otherwise, I would think we'd want to go with <A,P> or something like that.

If you think A/P is more readable, happy to change. I chose what I did because I kept messing up the letters, otherwise.

let's not block this PR on it. It's a simple thing to change in the future.

sdk/metrics/build.gradle.kts

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/GaugeData.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/PointData.java

jkwatson · 2021-06-28T21:27:20Z

I will admit I did not look at every single converted assertion, but this looks pretty good. Thanks for all the effort! Just a couple tiny nits and one question about generic type naming.

jsuereth and others added 2 commits June 23, 2021 20:21

Update java metrics data model and create assertion based testing lib…

ef21ce3

…rary.

Update existing SDK for Label => Attribute in data model

4f538e4

- Finish wiring through changes to all code/tests. - DOES NOT touch API - Update existing tests to use new testing library to be sensitive to attribute vs. label hashing alterations + equality of the metrics.data package types.

jsuereth requested review from anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, Oberon00, pavolloffay, thisthat and tylerbenson as code owners June 26, 2021 18:23

jsuereth mentioned this pull request Jun 27, 2021

Wire Exemplars into the metrics.data package #3353

Merged

jsuereth added 3 commits June 26, 2021 22:40

Fix javadoc breakages

d584b54

Fix JMH compile

9d8cefb

Finish javadoc

f31686f

anuraaga approved these changes Jun 28, 2021

View reviewed changes

Fixes from review

8c11aa8

pirgeo reviewed Jun 28, 2021

View reviewed changes

Remove botched file.

36e8bc2

jkwatson reviewed Jun 28, 2021

View reviewed changes

sdk/metrics/build.gradle.kts Outdated Show resolved Hide resolved

jkwatson reviewed Jun 28, 2021

View reviewed changes

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/GaugeData.java Outdated Show resolved Hide resolved

jkwatson reviewed Jun 28, 2021

View reviewed changes

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/PointData.java Outdated Show resolved Hide resolved

Fixes from review.

d521236

jkwatson approved these changes Jul 1, 2021

View reviewed changes

jkwatson merged commit b12a57e into open-telemetry:main Jul 1, 2021

jsuereth mentioned this pull request Aug 19, 2021

Update Metrics to latest specification. #3518

Closed

32 tasks

sullis mentioned this pull request Dec 19, 2021

Temurin JDK #4011

Merged

sullis mentioned this pull request Dec 19, 2021

use Eclipse Temurin JDK docker image #4012

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Metrics SDK "data' package to Attributes #3352

Migrate Metrics SDK "data' package to Attributes #3352

jsuereth commented Jun 26, 2021

anuraaga left a comment

anuraaga Jun 28, 2021

jsuereth Jun 28, 2021

anuraaga Jun 28, 2021

jsuereth Jun 28, 2021

anuraaga Jun 28, 2021

jsuereth Jun 28, 2021

anuraaga Jun 28, 2021

jsuereth Jun 28, 2021

jkwatson Jun 28, 2021

anuraaga Jun 28, 2021

jsuereth Jun 28, 2021

jkwatson Jun 28, 2021

pirgeo left a comment

jsuereth commented Jun 28, 2021

jkwatson Jun 28, 2021

jsuereth Jun 28, 2021

jkwatson Jul 1, 2021

jkwatson commented Jun 28, 2021

Migrate Metrics SDK "data' package to Attributes #3352

Migrate Metrics SDK "data' package to Attributes #3352

Conversation

jsuereth commented Jun 26, 2021

anuraaga left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirgeo left a comment

Choose a reason for hiding this comment

jsuereth commented Jun 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Jun 28, 2021