Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finish wiring exemplars into metrics SDK #3599

Merged
merged 24 commits into from
Sep 24, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3edcd58
Initial implementation of simple ExemplarReservoirs
jsuereth Sep 10, 2021
e82f67f
Remove explicit locks for mild performance boost.
jsuereth Sep 13, 2021
1923c3b
Create a new random holder abstraction.
jsuereth Sep 14, 2021
bff4e35
Add api diff + fix silly naming thing.
jsuereth Sep 15, 2021
fdd7a0a
Merge remote-tracking branch 'origin/main' into wip-exemplar-reservoi…
jsuereth Sep 15, 2021
3097982
Move random hodler to be Supplier, other updates from code review.
jsuereth Sep 16, 2021
3e0850e
Fixes from review.
jsuereth Sep 16, 2021
0d0179b
Fix bytebuddy issue with mocking.
jsuereth Sep 16, 2021
357061b
Expose aggregation configuration and wire in exemplar sampler
jsuereth Sep 10, 2021
abb302d
Fix merge conflict.
jsuereth Sep 16, 2021
0f79ce9
Wire exemplar filter config into autoconfigure module.
jsuereth Sep 16, 2021
142628f
Expose aggregation configuration and wire in exemplar sampler
jsuereth Sep 10, 2021
22c60f8
Fix merge conflict.
jsuereth Sep 16, 2021
a2f22bb
Wire exemplar filter config into autoconfigure module.
jsuereth Sep 16, 2021
45d6566
Migrate exemplar reservoir instantiation into aggregations. Start mi…
jsuereth Sep 18, 2021
de511cb
Merge branch 'wip-exemplar-complete' of https://github.com/jsuereth/o…
jsuereth Sep 18, 2021
b63f049
Small cleanup.
jsuereth Sep 18, 2021
620603c
Fixes from review (and report of clsasloader fixes from other laptop.
jsuereth Sep 22, 2021
e732205
Migrate to null-object pattern.
jsuereth Sep 22, 2021
a2a34fb
Merge remote-tracking branch 'origin/main' into wip-exemplar-complete
jsuereth Sep 23, 2021
7ee88f9
Add internal boilerplate.
jsuereth Sep 23, 2021
2e0a585
Run spotless.
jsuereth Sep 23, 2021
0b9a0f4
Fix toString of aggregation values.
jsuereth Sep 24, 2021
e1d1565
Fix ==
jsuereth Sep 24, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions sdk-extensions/autoconfigure/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ These properties can be used to control the maximum size of recordings per span.
| otel.span.event.count.limit | OTEL_SPAN_EVENT_COUNT_LIMIT | The maximum number of events per span. Default is `128`. |
| otel.span.link.count.limit | OTEL_SPAN_LINK_COUNT_LIMIT | The maximum number of links per span. Default is `128` |

## Exemplars

| System property | Environment variable | Description |
|--------------------------|--------------------------|-----------------------------------------------------------------------------------|
| otel.metrics.exemplar.filter | OTEL_METRICS_EXEMPLAR_FILTER | The filter for exemplar sampling. Can be `NONE`, `ALL` or `WITH_SAMPLED_TRACE`. Default is `WITH_SAMPLED_TRACE`.|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this PR since we're correctly reflecting the spec, but all of our other enum-type SDK config uses lowercase, not uppercase, would be nice if it can be fixed @jsuereth I don't know if we have time before the release though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a reasonable thing to fix. If all other spec uses lower case, that's an easy bugfix. We're only in feature-freeze right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's ok, I'll fix this in a follow up PR once this is submitted.


## Interval metric reader

| System property | Environment variable | Description |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import io.opentelemetry.sdk.autoconfigure.spi.metrics.SdkMeterProviderConfigurer;
import io.opentelemetry.sdk.metrics.SdkMeterProvider;
import io.opentelemetry.sdk.metrics.SdkMeterProviderBuilder;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarFilter;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.SdkTracerProvider;
import java.util.ServiceLoader;
Expand Down Expand Up @@ -90,6 +91,24 @@ private static void configureMeterProvider(Resource resource, ConfigProperties c

SdkMeterProviderBuilder meterProviderBuilder = SdkMeterProvider.builder().setResource(resource);

// Configure default exemplar filters.
String exemplarFilter = config.getString("otel.metrics.exemplar.filter");
if (exemplarFilter == null) {
exemplarFilter = "WITH_SAMPLED_TRACE";
}
switch (exemplarFilter) {
case "NONE":
meterProviderBuilder.setExemplarFilter(ExemplarFilter.neverSample());
break;
case "ALL":
meterProviderBuilder.setExemplarFilter(ExemplarFilter.alwaysSample());
break;
case "WITH_SAMPLED_TRACE":
default:
meterProviderBuilder.setExemplarFilter(ExemplarFilter.sampleWithTraces());
break;
}

for (SdkMeterProviderConfigurer configurer :
ServiceLoader.load(SdkMeterProviderConfigurer.class)) {
configurer.configure(meterProviderBuilder, config);
Expand Down
2 changes: 2 additions & 0 deletions sdk/metrics/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,6 @@ dependencies {
testImplementation(project(":sdk:metrics-testing"))
testImplementation(project(":sdk:testing"))
testImplementation("com.google.guava:guava")

jmh(project(":sdk:trace"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to put the benchmarks in sdk:all if possible this looks like an unexpected coupling of the SDK projects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JMH for trace also uses metrics sdk right now: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk/trace/build.gradle.kts#L37

This is in-line with our status-quo, but if you want to change that, I'd suggest outside this PR and we do it consistently.

}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.Tracer;
import java.util.concurrent.TimeUnit;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
Expand All @@ -18,6 +20,7 @@
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.TearDown;
import org.openjdk.jmh.annotations.Threads;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.ThreadParams;
Expand All @@ -37,16 +40,29 @@ public static class ThreadState {
@Param MetricsTestOperationBuilder opBuilder;

MetricsTestOperationBuilder.Operation op;
Span span;
io.opentelemetry.context.Scope contextScope;
final Attributes sharedLabelSet = Attributes.builder().put("KEY", "VALUE").build();
Attributes threadUniqueLabelSet;

@Setup
@SuppressWarnings("MustBeClosedChecker")
public void setup(ThreadParams threadParams) {
Meter meter = sdk.getMeter();
Tracer tracer = sdk.getTracer();
span = tracer.spanBuilder("benchmark").startSpan();
// We suppress warnings on closing here, as we rely on tests to make sure context is closed.
contextScope = span.makeCurrent();
op = opBuilder.build(meter);
threadUniqueLabelSet =
Attributes.builder().put("KEY", String.valueOf(threadParams.getThreadIndex())).build();
}

@TearDown
public void tearDown(ThreadParams threadParms) {
contextScope.close();
span.end();
}
}

@Benchmark
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.opentelemetry.api.metrics.LongCounter;
import io.opentelemetry.api.metrics.LongHistogram;
import io.opentelemetry.api.metrics.Meter;
import java.util.concurrent.ThreadLocalRandom;

/**
* This enum allows for iteration over all of the operations that we want to benchmark. To ensure
Expand Down Expand Up @@ -80,12 +81,13 @@ public void performBound() {

@Override
public void perform(Attributes labels) {
metric.record(5.0d, labels);
// We record different values to try to hit more areas of the histogram buckets.
metric.record(ThreadLocalRandom.current().nextDouble(0, 20_000d), labels);
}

@Override
public void performBound() {
boundMetric.record(5.0d);
boundMetric.record(ThreadLocalRandom.current().nextDouble(0, 20_000d));
}
};
}),
Expand All @@ -103,12 +105,12 @@ public void performBound() {

@Override
public void perform(Attributes labels) {
metric.record(5L, labels);
metric.record(ThreadLocalRandom.current().nextLong(0, 20_000L), labels);
}

@Override
public void performBound() {
boundMetric.record(5L);
boundMetric.record(ThreadLocalRandom.current().nextLong(0, 20_000L));
}
};
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@

import io.opentelemetry.api.metrics.Meter;
import io.opentelemetry.api.metrics.MeterProvider;
import io.opentelemetry.api.trace.Tracer;
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarFilter;
import io.opentelemetry.sdk.resources.Resource;
import io.opentelemetry.sdk.trace.SdkTracerProvider;
import io.opentelemetry.sdk.trace.samplers.Sampler;

@SuppressWarnings("ImmutableEnumChecker")
public enum TestSdk {
Expand All @@ -19,6 +23,18 @@ Meter build() {
return MeterProvider.noop().get("io.opentelemetry.sdk.metrics");
}
}),
SDK_NO_EXEMPLARS(
new SdkBuilder() {
@Override
Meter build() {
return SdkMeterProvider.builder()
.setClock(Clock.getDefault())
.setResource(Resource.empty())
.setExemplarFilter(ExemplarFilter.neverSample())
.build()
.get("io.opentelemetry.sdk.metrics");
}
}),
SDK(
new SdkBuilder() {
@Override
Expand All @@ -41,7 +57,18 @@ public Meter getMeter() {
return sdkBuilder.build();
}

public Tracer getTracer() {
return sdkBuilder.buildTracer();
}

private abstract static class SdkBuilder {
abstract Meter build();

protected Tracer buildTracer() {
return SdkTracerProvider.builder()
.setSampler(Sampler.alwaysOn())
.build()
.get("io.opentelemetry.sdk.metrics");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.internal.ComponentRegistry;
import io.opentelemetry.sdk.metrics.data.MetricData;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarSampler;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarFilter;
import io.opentelemetry.sdk.metrics.export.MetricProducer;
import io.opentelemetry.sdk.metrics.internal.state.MeterProviderSharedState;
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistry;
Expand Down Expand Up @@ -42,9 +42,9 @@ public final class SdkMeterProvider implements MeterProvider, MetricProducer {
private final MeterProviderSharedState sharedState;

SdkMeterProvider(
Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarSampler exemplarSampler) {
Clock clock, Resource resource, ViewRegistry viewRegistry, ExemplarFilter exemplarFilter) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I like Filter a lot better since we apply reservoir sampling after it anyways. Perhaps it allows renaming what we have as ExemplarReservoir to ExemplarFilter :-D

this.sharedState =
MeterProviderSharedState.create(clock, resource, viewRegistry, exemplarSampler);
MeterProviderSharedState.create(clock, resource, viewRegistry, exemplarFilter);
this.registry =
new ComponentRegistry<>(
instrumentationLibraryInfo -> new SdkMeter(sharedState, instrumentationLibraryInfo));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import io.opentelemetry.api.metrics.GlobalMeterProvider;
import io.opentelemetry.sdk.common.Clock;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarSampler;
import io.opentelemetry.sdk.metrics.exemplar.ExemplarFilter;
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistry;
import io.opentelemetry.sdk.metrics.internal.view.ViewRegistryBuilder;
import io.opentelemetry.sdk.metrics.view.InstrumentSelector;
Expand All @@ -25,7 +25,7 @@ public final class SdkMeterProviderBuilder {
private Resource resource = Resource.getDefault();
private final ViewRegistryBuilder viewRegistryBuilder = ViewRegistry.builder();
// Default the sampling strategy.
private ExemplarSampler exemplarSampler = ExemplarSampler.builder().build();
private ExemplarFilter exemplarFilter = ExemplarFilter.sampleWithTraces();

SdkMeterProviderBuilder() {}

Expand Down Expand Up @@ -54,12 +54,12 @@ public SdkMeterProviderBuilder setResource(Resource resource) {
}

/**
* Assign an {@link ExemplarSampler} for all metrics created by Meters.
* Assign an {@link ExemplarFilter} for all metrics created by Meters.
*
* @return this
*/
public SdkMeterProviderBuilder setExemplarSampler(ExemplarSampler sampler) {
this.exemplarSampler = sampler;
public SdkMeterProviderBuilder setExemplarFilter(ExemplarFilter filter) {
this.exemplarFilter = filter;
return this;
}

Expand Down Expand Up @@ -118,6 +118,6 @@ public SdkMeterProvider buildAndRegisterGlobal() {
* @see GlobalMeterProvider
*/
public SdkMeterProvider build() {
return new SdkMeterProvider(clock, resource, viewRegistryBuilder.build(), exemplarSampler);
return new SdkMeterProvider(clock, resource, viewRegistryBuilder.build(), exemplarFilter);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ public static ExemplarReservoir noSamples() {

/** Wraps a {@link ExemplarReservoir} with a measurement pre-filter. */
public static ExemplarReservoir filtered(ExemplarFilter filter, ExemplarReservoir original) {
// Optimisation on memory usage.
if (filter == ExemplarFilter.neverSample()) {
return ExemplarReservoir.noSamples();
}
return new FilteredExemplarReservoir(filter, original);
}

Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ public FixedSizeExemplarReservoir(Clock clock, int size, Supplier<Random> random

@Override
protected int reservoirIndexFor(double value, Attributes attributes, Context context) {
// Purposefuly truncate here.
int count = (int) numMeasurements.sum() + 1;
int count = numMeasurements.intValue() + 1;
int index = this.randomSupplier.get().nextInt(count > 0 ? count : 1);
numMeasurements.increment();
if (index < maxSize()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@
*/
@Immutable
public interface Aggregator<T> {
/**
* Returns the empty aggregator, an aggregator that never records measurements or reports values.
*/
static Aggregator<Void> empty() {
return EmptyAggregator.INSTANCE;
}
/**
* Returns a new {@link AggregatorHandle}. This MUST by used by the synchronous to aggregate
* recorded measurements during the collection cycle.
Expand Down
Loading