Swap old Metrics API for currently specified Metrics API #3423

jsuereth · 2021-07-24T21:17:18Z

Implement, as best as possible, the latest version of the metrics API specification, with feedback from Java SiG from the prototype.
Wire the new API into all other SDK usages of the API.
This PR touches the SDK as little as possible. It does the LEAST amount of renaming, rewiring and reworking to get things working. This is only meant to be an API swap, with a LOT of follow on PRs to come (based on the prototype). This leaves a lot of wonky behavior such as:
- The SDK references labels everywhere, including a LabelsProcessor for views. This will be refactored to match the SDK specifcaiton in follow-up PR.
- The SDK currently has "value observer" and "value recorder" instead of "gauge" and "histogram" instruments. We keep those in the SDK to minimize the diff-set to view, but they will be cleaned up in a follow on PR.
- The Histogram instrument is producing Summary metrics right now. This will be fixed when the new SDK implementation lands.
- The InstrumentDescriptor API has NOT BEEN touched, so instrument names are out of date. This will also be fixed in a follow up PR.

Passing off to other cmoputer.

codecov · 2021-07-25T00:07:15Z

Codecov Report

Merging #3423 (a0d4252) into main (20f872d) will decrease coverage by 0.29%.
The diff coverage is 92.09%.

❗ Current head a0d4252 differs from pull request most recent head 0413b69. Consider uploading reports for the commit 0413b69 to get more accurate results

@@             Coverage Diff              @@
##               main    #3423      +/-   ##
============================================
- Coverage     90.94%   90.65%   -0.30%     
+ Complexity     3258     3194      -64     
============================================
  Files           369      362       -7     
  Lines         10035     9904     -131     
  Branches       1019      998      -21     
============================================
- Hits           9126     8978     -148     
- Misses          597      613      +16     
- Partials        312      313       +1

Impacted Files	Coverage Δ
.../opentelemetry/sdk/metrics/AbstractInstrument.java	`72.72% <ø> (-15.28%)`	⬇️
...try/sdk/metrics/AbstractSynchronousInstrument.java	`100.00% <ø> (ø)`
...pentelemetry/sdk/metrics/DoubleSumObserverSdk.java	`100.00% <ø> (ø)`
...emetry/sdk/metrics/DoubleUpDownSumObserverSdk.java	`100.00% <ø> (ø)`
...opentelemetry/sdk/metrics/InstrumentProcessor.java	`95.00% <ø> (ø)`
.../opentelemetry/sdk/metrics/LongSumObserverSdk.java	`100.00% <ø> (ø)`
...elemetry/sdk/metrics/LongUpDownSumObserverSdk.java	`100.00% <ø> (ø)`
...io/opentelemetry/sdk/metrics/SdkMeterProvider.java	`100.00% <ø> (ø)`
...s/aggregator/AbstractMinMaxSumCountAggregator.java	`100.00% <ø> (ø)`
...entelemetry/sdk/metrics/aggregator/Aggregator.java	`0.00% <ø> (ø)`
... and 47 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 20f872d...0413b69. Read the comment docs.

… locally.

anuraaga

Just nits, all-in-all looks good

api/metrics/src/main/java/io/opentelemetry/api/metrics/BoundDoubleCounter.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/BoundDoubleHistogram.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/Counter.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/GlobalMeterProvider.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/BoundDoubleCounter.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongCounterBuilder.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongUpDownCounterBuilder.java

anuraaga · 2021-07-26T05:27:05Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/MeterProvider.java

   *
   * @param instrumentationName The name of the instrumentation library, not the name of the
   *     instrument*ed* library.
-   * @return a tracer instance.


anuraaga · 2021-07-26T05:29:06Z

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

  /** Returns an assertion for {@link DoubleHistogramPointData}. */
  public static DoubleHistogramPointDataAssert assertThat(DoubleHistogramPointData point) {
    return new DoubleHistogramPointDataAssert(point);
  }

+  /** Returns an assertion for {@link DoubleSummaryPointData}. */
+  public static DoubleSummaryPointDataAssert assertThat(DoubleSummaryPointData point) {


api/metrics/src/main/java/io/opentelemetry/api/metrics/DoubleCounterBuilder.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongHistogramBuilder.java

hdost · 2021-07-27T09:22:34Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongUpDownCounter.java

  /**
-   * Adds the given {@code increment} to the current value.
+   * Records a value.


Not sure this one makes sense to change. same with other add function comment change.

I went for consistency across instruments here. There's an 'oddity' here where users can actually use aggregators to change what happens when recording values. E.g. by default this would add an increment to current value, but you can use a View to convert this to a histogram or other behavior, which is why I went with record a value consistently across instruments.

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongUpDownCounterBuilder.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/BoundDoubleHistogram.java

jkwatson · 2021-08-03T18:03:53Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/BoundDoubleUpDownCounter.java


-  @Override
  void unbind();


I think all the unbind()s are going to need javadoc without the super interface.

Added, copying previous javadoc. Kinda unhappy with both bind + unbind's documentation on why/how they exists.

Thinking about discussing how (theoretically) if you only use BOUND instruments you should never see memory allocations in the hot path of your applications as, AFAIK, this is a guarantee from the metrics SDK. However, this is just the API so that guarantee isn't assured, only possible. WDYT?

yeah, we can't guarantee this in the API javadoc, unfortunately.

jkwatson · 2021-08-03T18:16:41Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/DoubleUpDownCounterBuilder.java

+   *
+   * @param callback A state-capturing callback used to observe values on-demand.
+   */
+  void buildWithCallback(Consumer<ObservableDoubleMeasurement> callback);


here's the case with both sync and async in the same builder. :)

api/metrics/src/main/java/io/opentelemetry/api/metrics/DoubleUpDownCounterBuilder.java

api/metrics/src/main/java/io/opentelemetry/api/metrics/GlobalMeterProvider.java

jkwatson · 2021-08-03T18:35:07Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/ObservableMeasurement.java

+ *
+ * <p>see {@link ObservableDoubleMeasurement} or {@link ObservableLongMeasurement}.
+ */
+public interface ObservableMeasurement {}


with no methods and no types does this have enough value to keep?

It helped in the SDK implementation. I can remove it, but you don't see the code which uses this yet.

...ters/otlp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/OtlpGrpcSpanExporter.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/AbstractInstrumentBuilder.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/view/InstrumentSelector.java

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

jkwatson · 2021-08-03T19:47:18Z

Just a few minor comments. Looks good! Thanks!

Co-authored-by: John Watson <jkwatson@gmail.com>

jkwatson

Approved, pending API discussion and a tiny capitalization nit.

anuraaga · 2021-08-05T07:17:26Z

@jkwatson I think it's ok to merge first and follow up on the API discussion given this is an alpha artifact and the PR is huge. What do you think?

jkwatson · 2021-08-05T15:09:09Z

@jkwatson I think it's ok to merge first and follow up on the API discussion given this is an alpha artifact and the PR is huge. What do you think?

This actually needs a rebase, since there have been additional API usages added since it was last rebased against main.

jack-berg · 2021-08-05T15:08:43Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/GlobalMeterProvider.java

-  public static MeterBuilder meterBuilder(String instrumentationName) {
-    return get().meterBuilder(instrumentationName);
+  public static void set(MeterProvider provider) {
+    globalMeterProvider = (provider == null) ? NoopMeterProvider.getInstance() : provider;


Equating null = NoopMeterProvider.getInstance() is strange IMO.

I think this should return or throw if null is passed. Throwing is inconsistent with current behavior, so I say return.

Existing behavior is to return a Noop meter provider on "get" if the meter provider is null. This just short-circuits that on the set side. i.e this is not a behavioral change.

Additionally, this entire class will disappear once metrics stabilize.

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongUpDownCounterBuilder.java

jack-berg · 2021-08-05T15:14:58Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/Meter.java

- *
- * <p>TODO: Update comment.
+ * <p>A Meter is generally associated with an instrumentation library, e.g. "I monitor apache
+ * httpclient".
 */
 @ThreadSafe
 public interface Meter {


The async instruments are accessible via the buildWithCallback methods from the respective builders, rather than from the Meter.

Is this allowed according to the spec?

I would say so. You still get the builder via the Meter. The spec isn't supposed to precisely define the language-specific APIs, but the capabilities that must be provided.

This was deemed a Java-specific deviation to abide by the common builder pattern in Java. I like the simplicity it acheived, but it is something we should discuss and can migrate to having separate builders by-async type instead of a builder per-metric-type and then having the async builder + sync builder coexist.

jack-berg · 2021-08-05T15:24:27Z

api/metrics/src/main/java/io/opentelemetry/api/metrics/LongUpDownCounterBuilder.java

+   *
+   * @param callback A state-capturing callback used to observe values on-demand.
+   */
+  void buildWithCallback(Consumer<ObservableLongMeasurement> callback);


I'd like to see "observable" somehow incorporated into the naming convention of methods that access async instruments. Its recommended in the spec and I think it better clarifies the async distinction than "callback".

Maybe buildObservable?

@jkwatson @anuraaga for comment. This was a specific deviation to fit the java "builder" convention, so I defer to Java SiG here.

The spec calls this an "observable" I think? In that case, probably migrating to that verbiage would be helpful. I don't want to hold up this PR for that change, though.

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeterProvider.java

jkwatson · 2021-08-05T20:58:41Z

I'm going to merge this, so we can build from it. Thanks, @jsuereth !

jsuereth and others added 10 commits July 24, 2021 11:01

Update the API to the latest SDK specification.

a916fc2

API updates to other sdk areas.

f360976

First half of SDK updates for API.

b054047

Passing off to other cmoputer.

Get SDK compiling again.

707def4

Get tests compiling again (and failing).

3fd9a92

Fix bad copy-paste error.

50e32d0

Get all SDK tests passing.

dc76b6f

More fixes to builds across SDK impls.

8a14aad

Remove unecessary publics

ba09220

more fixes for new API.

e114c0d

jsuereth requested review from anuraaga, arminru, bogdandrutu, carlosalberto and jkwatson as code owners July 24, 2021 21:17

jsuereth requested a review from a user July 24, 2021 21:17

jsuereth requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners July 24, 2021 21:17

jsuereth and others added 3 commits July 24, 2021 17:20

Fix merge conflict.

f718d23

spotless fixes.

161c885

Make tests for metric points order independent.

41be16f

jsuereth added 5 commits July 25, 2021 10:53

Restore readme.

314efbf

Fix readmes.

906fed9

Add noop meter provider tests for code coverage.

2cbad26

Add code coverage for assertion library.

06ed8e2

Fix wierd test failure that gradle cache is preventing me from seeing…

4ae6c66

… locally.

anuraaga reviewed Jul 26, 2021

View reviewed changes

Switch from atomic ref to volatile.

08a36b4

anuraaga approved these changes Jul 30, 2021

View reviewed changes

hdost reviewed Jul 30, 2021

View reviewed changes

Fixes from review.

9b898ac