MetricReader and Multi-export features #3578

jsuereth · 2021-09-05T17:43:15Z

Adds MetricReader interface, and expands Java SDK to allow multiple exporters to be registered.

Implements multi-export for SYNCHRONOUS INSTRUMENTS. ASYNC work as-is but are currently broken for DELTA aggregation (in a trivial to reproduce way).
Adds bare minimum tests or evolution of tests.
Implements the following exporters:
- OTLP (as-is)
- InMemory (shifted to use MetricReader)
- Prometheus (shifted to use MetricReader)
- PeriodMetricReader (shifted from IntervalMetricReader + adapted to MetricReader interface)
Updates autoconf to use new style of registration (albeit this can be improved).

Implementation notes

The key interfaces are as follows:

interface MetricProducer {
   Collection<MetricData> collectAllMetrics();
}
interface MetricReader {
    CompletableResultCode flush();
    CompletableResultCode shutdown();
}
interface MetricReaderFactory {
      MetricReaer apply(MetricProducer producer);
}
interface MetricExporter {
   CompletableResultCode export(Collection<MetricData> batch);
   CompletableResultCode flush();
   CompletableResultCode shutdown();
}
interface SdkMeterProviderBuilder {
   SdkMetricProviderBuilder registerMetricReader(MetricReaderFactory readerFactory);
}

Effectively this accomplishes the following:

Shutdown behavior, similar to TracerProvider, is available directly on the MeterProvider.
SdkMeterProvide can know (with 100% certainty) how many readers there are and can influence their lifecycle (via flush/shutdown).

Tracking metric collection

We introduced CollectionHandle which provides bit-set functionality (and integer equality) for tracking metric reader and reseting mutable interval-based state based on metric reader. Effectively every metric reader is given a CollectionHandle which is a bit-value within the integer range (limiting us to 32 metric readers per sdk).

SynchronousMetricStorage

DeltaMeasurementStorage
- We have a mechanism (on every collection call) to grab current delta measurements from synchronous instruments.
- Every measurement remembers which collection handles have read the measurement
- This storage is automatically cleaned once a measurement has been read by all handles.
CumualtiveMetricStorage
- We have another mechanism to remember the last reported metric point per-collector

The basic algorithm is:

Go grab recent delta measurements.
Merge all unread delta measurements for this collector
Merge delta-measurement w/ previous cumulative, if reporting cumulative.
Perform garbage collection

AsynchronousMetricStorage

This will be updated to track previous values for "mergeDiff" of Sum aggregation (or log errors for DELTA temporality depending on specification).

reyang · 2021-09-07T15:34:12Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MetricExporterConfiguration.java

- if (exportInterval != null) {
- readerBuilder.setExportIntervalMillis(exportInterval.toMillis());
+ if (exportInterval == null) {
+ // TODO: What default for reading?


In the current spec PR - "The default value is 5000 (milliseconds)."

Wow, that's quite.... fast. The default in OpenCensus was 1 minute. Should we slow it down a bit?

I don't have strong opinion, I got the value (5000ms) from the tracing SDK spec for consistency (and I think consistency here is not something super important).

Note the difference - for span processor, the default 5000ms is the delay between two consecutive exports, so the actual interval is something like 5000ms + exporting time, in metrics we are taking different approach by using a fixed schedule (so the delay is 5000ms - exporting time).

I think there's also a huge difference in batch size + aggregation between metrics + traces.

Traces are known to be high-frequency + highly sampled. Metrics we're leveraging aggregation to limit bandwidth, so the export interval should be different. Given what I know of our customers + vendor requests around metrics, I think a default of 1 minute should accommodate most users (and be too fast for some).

I know that for Microsoft most of the systems use either 20 seconds or 1 minute so I'm totally fine with 1 min.

I've updated the PR to change it to 60 seconds open-telemetry/opentelemetry-specification@68f4fdc.

Hm. Having to wait a full minute, on top of ingest time for the backend is a very long time to wait for metrics to appear on new deployments. What's the harm in having it be faster?

In OpenCensus there was the agility to have a different delay between when the first metrics are exported and the average time between each export.

My thinking on this is as follows:

For short-lived processes, the shutdown hook + export is crucial, and one of the benefits of push-metrics.

For most other metrics, one of the reasons to use metrics OVER logs or traces is because you can greatly shrink the sheer size of the data. If we report too frequently we put more pressure on backends to collapse the data or risk being very expensive for users.

For scenarios where we're serving HTTP traffic, if we send too frequently, we're unlikely to have explored/opened all metric-streams within the reporting interval. This actually has memory/gc implications for the implementation.

For initial-startup "is it working" scenarios, it probably makes sense to default a quick fire of metrics

TL;DR:

Our default should balance "Most metric streams have a point recorded during this time" and "Users don't run a huge bill on storage", "Users can see changes live" and "Nice for debugging"

I think the "is it working on startup" use case is enough to shorten time-to-initial report, but not the on-going reporting interval. There's some other nice benefits we may be able to get here like restart-loop detection. However I think the cost of reporting too frequently is a real thing. Users should opt-in if they want higher resolution.

It would be really cool to have a dynamic system that would send metrics more frequently during unusual circumstances (like startup), but I have no idea how one might do that. :)

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

… SdkMeterProvider - InMemoryMetricReader is now a factory + reader - Hide register on SdkMeterProvider - Update all tests to make sure things pass.

…. Update all code around registration of readers and autoconfigure. Create new 'sdk noop' meter provider for when we have no readers configured.

jsuereth · 2021-09-18T18:04:49Z

@jkwatson @anuraaga This is ready for an "API" evaluation. Still some known issues in the implementation I'm working through, but would love thoughts on the API before I dive too much further.

jkwatson · 2021-09-20T15:49:34Z

...rters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusCollector.java

- private final MetricProducer metricProducer;
+public final class PrometheusCollector extends Collector
+ implements MetricReader, MetricReader.Factory {
+ private MetricProducer metricProducer;


volatile just to be sure?

Shouldn't be necessary as this will be filled out before the class is registered w/ prometheus.

We can add it, or I can document the reason it's ok not to be volatile here.

Added documentation on why, PTAL and let me know if you'd still like volatile here.

Hm. I read your description and I'm not convinced. :) The JMM is a tricky beast and visibility of non-final, non-volatiles is likely to trip one up at the worst possible time (in production). Is "shared with other threads" a concrete JMM thing for non-final, non-volatiles?

Can we just have PrometheusCollector.createFactory() which returns a MetricReaderFactory, not MetricReader? The factory can initialize the MetricReader with a concrete reference without needing to make the MetricReader mutable I think. createFactory could just return PrometheusCollector::new presuambly

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReader.java

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

jkwatson · 2021-09-20T17:01:59Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MetricExporterConfiguration.java

+ // Register the reader (which starts it), and use a global shutdown hook rather than
+ // interval reader specific.


Registering the reader with the builder doesn't start it. It doesn't get started until the SdkMeterProvider is actually built, correct?

Yeah, I'll update the comments.

jkwatson · 2021-09-20T17:03:51Z

...oconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/MetricExporterConfiguration.java

+ // TODO: Can we move this portion into the PrometheusCollector so shutdown on SdkMeterProvider
+ // will collapse prometheus too?


Not the config reading part, but the server creation/startup makes sense to move there, yes.

...io/opentelemetry/sdk/extension/incubator/trace/ExecutorServiceSpanProcessorCpuBenchmark.java

jkwatson · 2021-09-20T17:13:50Z

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

+import javax.annotation.Nullable;
+
+/**
+ * {@code SdkMeterProvider} implementation for {@link MeterProvider}.


nit: MeterProvider isn't imported here, so the javadoc doesn't resolve the link properly.

jkwatson · 2021-09-20T17:15:49Z

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

+ Supplier<CollectionHandle> handleSupplier = CollectionHandle.createSupplier();
+ for (MetricReader.Factory readerFactory : readerFactories) {
+ CollectionHandle handle = handleSupplier.get();
+ // TODO: handle failure in creation or just crash?
+ MetricReader reader = readerFactory.apply(new LeasedMetricProducer(handle));
+ collectors.add(handle);
+ readers.add(reader);
+ }


I don't much like doing all this work in the constructor. Can we move this stuff to an init method of some sort to be called after construction is complete?

Would this be called immediately after construction always anyway?

If we want immutable / final references to this config, it needs to be in the constructor. I moved it this way to avoid having a "register" method be publicly exposed the could dynamically adjust configuration (and require locks). We can shift this around, but having another method that mutates state feels just as bad as exposing "register" to me.

jkwatson · 2021-09-20T17:20:50Z

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

+
+ @Override
+ public CompletableResultCode shutdown() {
+ // TODO - prevent multiple calls.


or just ignore subsequent ones, returning the appropriate result code.

Right, do we have an example of that impl I can copy-paste?

heh. I looked through the code and found a variety of approaches, none of which look great. It will probably involve saving off the CompletableResultCode somewhere, and just returning it if it's already there.

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

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExporter.java

jsuereth · 2021-09-24T15:14:25Z

Not sure how to solve the codecov issues, it seems like it's not seeing much coverage on existing exporters + autoconfigure.

anuraaga · 2021-09-27T02:38:43Z

...rters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusCollector.java

+ }
+
+ /** Our implementation of the metric reader factory. */
+ private static class Factory implements MetricReaderFactory {


Not a big deal but looks like can be singleton

There's a refactoring (I can do in this PR or later) where we instantiate the HTTP server as part of the factory (configurable). Would you like that in this PR? I'll add a note in the meantime.

anuraaga · 2021-09-27T02:39:37Z

...rters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusCollector.java

@@ -21,20 +34,45 @@

 @Override
 public List<MetricFamilySamples> collect() {
+ if (metricProducer == null) {


Can remove this and add requireNonNull in the constructor instead (construction of SDK classes can generally throw)

Ah, this shouldn't remain, after the refactoring anyway. It "can't" be null if all our @Nullable annotations are accurage.

anuraaga · 2021-09-27T02:41:41Z

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

+ private final ComponentRegistry<SdkMeter> registry;
+ private final MeterProviderSharedState sharedState;
+
+ // These are *only* mutated in our constructor, and safe to use concurrently after construction.


We can model this in code better by initializing the variables in the collector as well

anuraaga · 2021-09-27T02:41:53Z

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

+
+ private static final Logger LOGGER = Logger.getLogger(DefaultSdkMeterProvider.class.getName());
+ static final String DEFAULT_METER_NAME = "unknown";
+ private final ComponentRegistry<SdkMeter> registry;


Suggested change

private final ComponentRegistry<SdkMeter> registry;

private final ComponentRegistry<SdkMeter> registry;

anuraaga · 2021-09-27T02:43:22Z

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

+ Collection<SdkMeter> meters = registry.getComponents();
+ List<MetricData> result = new ArrayList<>(meters.size());
+ for (SdkMeter meter : meters) {
+ result.addAll(meter.collectAll(handle, collectors, sharedState.getClock().now()));


anuraaga · 2021-09-27T03:01:35Z

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/testing/InMemoryMetricReader.java

+ * </code></pre>
+ */
+public class InMemoryMetricReader implements MetricReader, MetricReaderFactory {
+ // Note: we expect the `apply` method of `MetricReaderFactory` to be called


Even for testing think it's worth sticking to the same pattern we did to Prometheus (here it really should just be returning ::new :)

We can't, you loose access to the instantiated class if you do so, which means you never have access to collect metrics.

This is designed so you can directly call "collect" and extract metrics. (also, should this class be in the metric-testing artifact?)

anuraaga · 2021-09-27T03:03:49Z

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

+ * <p>All synchronous handles will be collected + reset during this method. Additionally cleanup
+ * related stale concurrent-map handles will occur. Any {@code null} measurements are ignored.
+ */
+ @GuardedBy("this")


I think this is supposed to be on fields, not on methods (and errorprone will ensure the marked fields are only accessed under said lock)

This did work for me in VisualStudio, but I can remove if you'd like.

Working means if a not synchronized method accesses the ArrayList without taking the lock it fails the build. Is vscode really able to understand that with this pattern? And is Gradle able to? The requirement is for Gradle to catch it, and if it can then I'm missing parts of the behavior of this annotation so happy to know that.

By the way, think I'm just waiting on this one to merge, don't want to get confused later

Or we can merge after filing an issue about it if it seems hairy, since it's about thread safety want to make sure it doesn't get lost.

Didn't realize this was still open (sorry soo many comments I was missing things). Just went with synchronized for now and we can rework the class internals over time (there's a few TODOs for it anyway).

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

anuraaga · 2021-09-27T03:08:52Z

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

+ }
+ }
+ // Now run a quick cleanup of deltas before returning.
+ unreportedDeltas.removeIf(delta -> delta.wasReadyByAll(collectors));


IIUC, one of the most significant changes from our current model is that rather than clearing all data every collection, we mark each time data has been read, and only after it's been read by all collectors do we clean up. Just want to confirm my understanding

Yes. We store semi-aggregated data every time any reader wants to collect metrics, then we reconstitute the full-delta per-reader and join w/ any cumulative storage before reporting. There's a design "markdown" from Reiley that kind of clarifies this, and I can take a crack at something. Hoping the new naming helps make this more clear.

Effectively the goal of this class is to get the Synchronous collection path (that's highly performance sensitive) into the reader thread as fast as possible. We don't care too much about reader contention because we don't expect significant numbers of reader threads.

A few improvements over time:

I'd like to pre-allocate delta storage and limit its size. That should be hidden in this class and would be per-metric.

I'd like to allow multiple reader "within interval" to avoid re-collecting synchronous metrics.

I'd like to join multiple MetricExporters into a single MetricReader if they all use periodic metric reader.

I can open tickets around these improvements, but wanted to make sure we're ok with how this code works now.

docs/apidiffs/current_vs_latest/opentelemetry-exporter-logging-otlp.txt

jkwatson · 2021-09-27T15:45:08Z

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExporter.java

 *
 * @return the result of the flush, which is often an asynchronous operation.
 */
- CompletableResultCode flush();
+ CompletableResultCode forceFlush();


I'm against renaming this method. It breaks our existing API compatibility guarantees and makes the method out of alignment with the SpanExporter interface. Is there a strong reason why we're adding the "force" to it?

I think maybe I'm misunderstanding some comments/conventions here.

I migrated all uses of flush to forceFlush to conform with SdkTracerProvider. It looks like exporters use flush and the SDK*Provider uses forceFlush.

I'll revert this specific change. Do we want SdkMeterProivider to exposes forceFlush or just flush? From my perspective each is equally good, so if flush was exposed in a stable API, happy to change this.

Original comment (I likely took way too far): #3578 (comment)

For clarity on what we have:

*Exporter has:

flush()

shutdown()

Sdk*Provider has:

forceFlush()

close()

shutdown()

If we're on board, I'll make sure metrics matches traces with that.

yeah, for good or ill, let's at least have the parallel interfaces have the same methods. I honestly don't remember why exporters got flush when the providers got forceFlush.

...rters/prometheus/src/main/java/io/opentelemetry/exporter/prometheus/PrometheusCollector.java

sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/PeriodicMetricReader.java

jkwatson

This seems good enough to merge and polish as we go.

…nto wip-multi-exporters

jsuereth and others added 5 commits September 5, 2021 13:27

Start of multi-exporter codebase.

d654410

Restore InMemoryExporter + fix javadocs.

9b77bc5

Quick javadoc fix

6b6357e

Fix some javadoc build failures and missing things.

f8af124

Fix bug with autoconfigure

fd448a9

jsuereth mentioned this pull request Sep 6, 2021

Update Metrics to latest specification. #3518

Closed

32 tasks

reyang reviewed Sep 7, 2021

View reviewed changes

reyang mentioned this pull request Sep 8, 2021

Add MetricReader interface open-telemetry/opentelemetry-specification#1888

Merged

jkwatson reviewed Sep 10, 2021

View reviewed changes

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

jsuereth added 4 commits September 12, 2021 09:43

Modify InMemoryMetricReader + register methods to allow for immutable…

e6b5281

… SdkMeterProvider - InMemoryMetricReader is now a factory + reader - Hide register on SdkMeterProvider - Update all tests to make sure things pass.

Merge remote-tracking branch 'otel/main' into wip-multi-exporters

abea187

Make metric-readers 'immutable' post-construction of SdkMeterProvider…

559cec5

…. Update all code around registration of readers and autoconfigure. Create new 'sdk noop' meter provider for when we have no readers configured.

Fix integration test.

b054e70

Fix jmh benchmarks for trace.

7c8de33

jsuereth changed the title ~~DRAFT - Working multi-exporter + MetricReader to evaluate spec proposal~~ DRAFT - MetricReader and Multi-export features Sep 18, 2021

jsuereth added 2 commits September 18, 2021 18:08

Fix jmh build.

b30d600

Fix broken in-memory exporter things, and Noop API issue.

8533e83