Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@lenin-jaganathan
lenin-jaganathan committed Aug 9, 2024
1 parent 56238b2 commit 0108fda
Show file tree
Hide file tree
Showing 21 changed files with 69 additions and 209 deletions.
10 changes: 5 additions & 5 deletions ...s/benchmarks-core/src/jmh/java/io/micrometer/benchmark/compare/CompareOTLPHistograms.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.Timer;
import io.micrometer.registry.otlp.AggregationTemporality;
import io.micrometer.registry.otlp.HistogramFlavour;
import io.micrometer.registry.otlp.HistogramFlavor;
import io.micrometer.registry.otlp.OtlpConfig;
import io.micrometer.registry.otlp.OtlpMeterRegistry;

Expand Down Expand Up @@ -198,8 +198,8 @@ public static class ExponentialHistogramCumulative {

OtlpConfig otlpConfig = new OtlpConfig() {
@Override
public HistogramFlavour histogramFlavour() {
return HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
public HistogramFlavor histogramFlavor() {
return HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
}

@Nullable
Expand Down Expand Up @@ -240,8 +240,8 @@ public AggregationTemporality aggregationTemporality() {
}

@Override
public HistogramFlavour histogramFlavour() {
return HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
public HistogramFlavor histogramFlavor() {
return HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
}

@Nullable
Expand Down
8 changes: 4 additions & 4 deletions ...meter/registry/otlp/HistogramFlavour.java → ...ometer/registry/otlp/HistogramFlavor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,22 @@
package io.micrometer.registry.otlp;

/**
* Histogram Flavour to be used while recording distributions,
* Histogram Flavor to be used while recording distributions,
*
* @see <a href=
* "https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk_exporters/otlp.md#additional-configuration">OTLP
* Configuration</a>
* @author Lenin Jaganathan
* @since 1.14.0
*/
public enum HistogramFlavour {
public enum HistogramFlavor {

EXPLICIT_BUCKET_HISTOGRAM, BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;

/**
* Converts a string to {@link HistogramFlavour} by using a case-insensitive matching.
* Converts a string to {@link HistogramFlavor} by using a case-insensitive matching.
*/
public static HistogramFlavour fromString(final String histogramPreference) {
public static HistogramFlavor fromString(final String histogramPreference) {
if (BASE2_EXPONENTIAL_BUCKET_HISTOGRAM.name().equalsIgnoreCase(histogramPreference)) {
return BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
}
Expand Down
22 changes: 11 additions & 11 deletions ...ations/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,27 +204,27 @@ default Map<String, String> headers() {

/**
* Histogram type to be preferred when histogram publishing is enabled. By default
* {@link HistogramFlavour#EXPLICIT_BUCKET_HISTOGRAM} is used for the supported
* meters. When this is set to
* {@link HistogramFlavour#BASE2_EXPONENTIAL_BUCKET_HISTOGRAM} and publishPercentiles
* are enabled {@link io.micrometer.registry.otlp.internal.Base2ExponentialHistogram}
* is used for recording distributions.
* {@link HistogramFlavor#EXPLICIT_BUCKET_HISTOGRAM} is used for the supported meters.
* When this is set to {@link HistogramFlavor#BASE2_EXPONENTIAL_BUCKET_HISTOGRAM} and
* publishPercentiles are enabled
* {@link io.micrometer.registry.otlp.internal.Base2ExponentialHistogram} is used for
* recording distributions.
* <p>
* Note: If specific SLO's are added as part of meters, this property is not honored
* and {@link HistogramFlavour#EXPLICIT_BUCKET_HISTOGRAM} is used for those meters.
* and {@link HistogramFlavor#EXPLICIT_BUCKET_HISTOGRAM} is used for those meters.
* </p>
* @return - histogram flavour to be used
* @return - histogram flavor to be used
*
* @since 1.14.0
*/
default HistogramFlavour histogramFlavour() {
return getEnum(this, HistogramFlavour.class, "histogramFlavour").orElseGet(() -> {
default HistogramFlavor histogramFlavor() {
return getEnum(this, HistogramFlavor.class, "histogramFlavor").orElseGet(() -> {
String histogramPreference = System.getenv()
.get("OTEL_EXPORTER_OTLP_METRICS_DEFAULT_HISTOGRAM_AGGREGATION");
if (histogramPreference != null) {
return HistogramFlavour.fromString(histogramPreference);
return HistogramFlavor.fromString(histogramPreference);
}
return HistogramFlavour.EXPLICIT_BUCKET_HISTOGRAM;
return HistogramFlavor.EXPLICIT_BUCKET_HISTOGRAM;
});
}

Expand Down
6 changes: 3 additions & 3 deletions ...try-otlp/src/main/java/io/micrometer/registry/otlp/OtlpCumulativeDistributionSummary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
class OtlpCumulativeDistributionSummary extends CumulativeDistributionSummary
implements StartTimeAwareMeter, OtlpHistogramSupport {

private final HistogramFlavour histogramFlavour;
private final HistogramFlavor histogramFlavor;

private final long startTimeNanos;

Expand All @@ -36,7 +36,7 @@ class OtlpCumulativeDistributionSummary extends CumulativeDistributionSummary
super(id, clock, distributionStatisticConfig, scale,
OtlpMeterRegistry.getHistogram(clock, distributionStatisticConfig, otlpConfig));
this.startTimeNanos = TimeUnit.MILLISECONDS.toNanos(clock.wallTime());
this.histogramFlavour = OtlpMeterRegistry.histogramFlavour(otlpConfig.histogramFlavour(),
this.histogramFlavor = OtlpMeterRegistry.histogramFlavor(otlpConfig.histogramFlavor(),
distributionStatisticConfig);
}

Expand All @@ -48,7 +48,7 @@ public long getStartTimeNanos() {
@Override
@Nullable
public ExponentialHistogramSnapShot getExponentialHistogramSnapShot() {
if (histogramFlavour == HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
if (histogramFlavor == HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
return ((Base2ExponentialHistogram) histogram).getLatestExponentialHistogramSnapshot();
}
return null;
Expand Down
6 changes: 3 additions & 3 deletions ...crometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpCumulativeTimer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@

class OtlpCumulativeTimer extends CumulativeTimer implements StartTimeAwareMeter, OtlpHistogramSupport {

private final HistogramFlavour histogramFlavour;
private final HistogramFlavor histogramFlavor;

private final long startTimeNanos;

OtlpCumulativeTimer(Id id, Clock clock, DistributionStatisticConfig distributionStatisticConfig,
PauseDetector pauseDetector, TimeUnit baseTimeUnit, OtlpConfig otlpConfig) {
super(id, clock, distributionStatisticConfig, pauseDetector, baseTimeUnit,
OtlpMeterRegistry.getHistogram(clock, distributionStatisticConfig, otlpConfig, baseTimeUnit));
this.histogramFlavour = OtlpMeterRegistry.histogramFlavour(otlpConfig.histogramFlavour(),
this.histogramFlavor = OtlpMeterRegistry.histogramFlavor(otlpConfig.histogramFlavor(),
distributionStatisticConfig);
this.startTimeNanos = TimeUnit.MILLISECONDS.toNanos(clock.wallTime());
}
Expand All @@ -48,7 +48,7 @@ public long getStartTimeNanos() {
@Override
@Nullable
public ExponentialHistogramSnapShot getExponentialHistogramSnapShot() {
if (histogramFlavour == HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
if (histogramFlavor == HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
return ((Base2ExponentialHistogram) histogram).getLatestExponentialHistogramSnapshot();
}
return null;
Expand Down
21 changes: 9 additions & 12 deletions ...micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpMeterRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,6 @@ protected TimeUnit getBaseTimeUnit() {
protected DistributionStatisticConfig defaultHistogramConfig() {
return DistributionStatisticConfig.builder()
.expiry(this.config.step())
.maxBucketCount(this.config.maxBucketCount())
.build()
.merge(DistributionStatisticConfig.DEFAULT);
}
Expand Down Expand Up @@ -393,25 +392,23 @@ static Histogram getHistogram(Clock clock, DistributionStatisticConfig distribut
static Histogram getHistogram(final Clock clock, final DistributionStatisticConfig distributionStatisticConfig,
final OtlpConfig otlpConfig, @Nullable final TimeUnit baseTimeUnit) {
// While publishing to OTLP, we export either Histogram datapoint (Explicit
// ExponentialBucket
// ExponentialBuckets
// or Exponential) / Summary
// datapoint. So, we will make the histogram either of them and not both.
// Though AbstractTimer/Distribution Summary prefers publishing percentiles,
// exporting of histograms over percentiles is preferred in OTLP.
if (distributionStatisticConfig.isPublishingHistogram()) {
if (HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM
.equals(histogramFlavour(otlpConfig.histogramFlavour(), distributionStatisticConfig))) {
if (HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM
.equals(histogramFlavor(otlpConfig.histogramFlavor(), distributionStatisticConfig))) {
Double minimumExpectedValue = distributionStatisticConfig.getMinimumExpectedValueAsDouble();
if (minimumExpectedValue == null) {
minimumExpectedValue = 0.0;
}

final int maxBucketCount = distributionStatisticConfig.getMaxBucketCount() != null
? distributionStatisticConfig.getMaxBucketCount() : otlpConfig.maxBucketCount();
return otlpConfig.aggregationTemporality() == AggregationTemporality.DELTA
? new DeltaBase2ExponentialHistogram(otlpConfig.maxScale(), maxBucketCount,
? new DeltaBase2ExponentialHistogram(otlpConfig.maxScale(), otlpConfig.maxBucketCount(),
minimumExpectedValue, baseTimeUnit, clock, otlpConfig.step().toMillis())
: new CumulativeBase2ExponentialHistogram(otlpConfig.maxScale(), maxBucketCount,
: new CumulativeBase2ExponentialHistogram(otlpConfig.maxScale(), otlpConfig.maxBucketCount(),
minimumExpectedValue, baseTimeUnit);
}

Expand All @@ -428,17 +425,17 @@ static Histogram getHistogram(final Clock clock, final DistributionStatisticConf
return NoopHistogram.INSTANCE;
}

static HistogramFlavour histogramFlavour(HistogramFlavour preferredHistogramFlavour,
static HistogramFlavor histogramFlavor(HistogramFlavor preferredHistogramFlavor,
DistributionStatisticConfig distributionStatisticConfig) {

final double[] serviceLevelObjectiveBoundaries = distributionStatisticConfig
.getServiceLevelObjectiveBoundaries();
if (distributionStatisticConfig.isPublishingHistogram()
&& preferredHistogramFlavour == HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM
&& preferredHistogramFlavor == HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM
&& (serviceLevelObjectiveBoundaries == null || serviceLevelObjectiveBoundaries.length == 0)) {
return HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
return HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM;
}
return HistogramFlavour.EXPLICIT_BUCKET_HISTOGRAM;
return HistogramFlavor.EXPLICIT_BUCKET_HISTOGRAM;
}

@Nullable
Expand Down
6 changes: 3 additions & 3 deletions ...-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpStepDistributionSummary.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@

class OtlpStepDistributionSummary extends AbstractDistributionSummary implements OtlpHistogramSupport {

private final HistogramFlavour histogramFlavour;
private final HistogramFlavor histogramFlavor;

private final LongAdder count = new LongAdder();

Expand All @@ -50,7 +50,7 @@ class OtlpStepDistributionSummary extends AbstractDistributionSummary implements
this.countTotal = new OtlpStepTuple2<>(clock, otlpConfig.step().toMillis(), 0L, 0.0, count::sumThenReset,
total::sumThenReset);
this.max = new StepMax(clock, otlpConfig.step().toMillis());
this.histogramFlavour = OtlpMeterRegistry.histogramFlavour(otlpConfig.histogramFlavour(),
this.histogramFlavor = OtlpMeterRegistry.histogramFlavor(otlpConfig.histogramFlavor(),
distributionStatisticConfig);
}

Expand Down Expand Up @@ -78,7 +78,7 @@ public double max() {

@Override
public ExponentialHistogramSnapShot getExponentialHistogramSnapShot() {
if (histogramFlavour == HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
if (histogramFlavor == HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
return ((Base2ExponentialHistogram) histogram).getLatestExponentialHistogramSnapshot();
}
return null;
Expand Down
6 changes: 3 additions & 3 deletions ...ons/micrometer-registry-otlp/src/main/java/io/micrometer/registry/otlp/OtlpStepTimer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

class OtlpStepTimer extends AbstractTimer implements OtlpHistogramSupport {

private final HistogramFlavour histogramFlavour;
private final HistogramFlavor histogramFlavor;

private final LongAdder count = new LongAdder();

Expand All @@ -54,7 +54,7 @@ class OtlpStepTimer extends AbstractTimer implements OtlpHistogramSupport {
countTotal = new OtlpStepTuple2<>(clock, otlpConfig.step().toMillis(), 0L, 0L, count::sumThenReset,
total::sumThenReset);
max = new StepMax(clock, otlpConfig.step().toMillis());
this.histogramFlavour = OtlpMeterRegistry.histogramFlavour(otlpConfig.histogramFlavour(),
this.histogramFlavor = OtlpMeterRegistry.histogramFlavor(otlpConfig.histogramFlavor(),
distributionStatisticConfig);
}

Expand Down Expand Up @@ -100,7 +100,7 @@ else if (histogram instanceof Base2ExponentialHistogram) {

@Override
public ExponentialHistogramSnapShot getExponentialHistogramSnapShot() {
if (histogramFlavour == HistogramFlavour.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
if (histogramFlavor == HistogramFlavor.BASE2_EXPONENTIAL_BUCKET_HISTOGRAM) {
return ((Base2ExponentialHistogram) histogram).getLatestExponentialHistogramSnapshot();
}
return null;
Expand Down
6 changes: 3 additions & 3 deletions ...ry-otlp/src/main/java/io/micrometer/registry/otlp/internal/Base2ExponentialHistogram.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
package io.micrometer.registry.otlp.internal;

import static io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBucket.EMPTY_EXPONENTIAL_BUCKET;
import static io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBuckets.EMPTY_EXPONENTIAL_BUCKET;

import java.util.Arrays;
import java.util.Collections;
Expand All @@ -28,7 +28,7 @@
import io.micrometer.core.instrument.distribution.Histogram;
import io.micrometer.core.instrument.distribution.HistogramSnapshot;
import io.micrometer.core.instrument.util.TimeUtils;
import io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBucket;
import io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBuckets;

/**
* A ExponentialHistogram implementation that compresses bucket boundaries using an
Expand Down Expand Up @@ -125,7 +125,7 @@ ExponentialHistogramSnapShot getCurrentValuesSnapshot() {
return (circularCountHolder.isEmpty() && zeroCount.longValue() == 0)
? DefaultExponentialHistogramSnapShot.getEmptySnapshotForScale(scale)
: new DefaultExponentialHistogramSnapShot(scale, zeroCount.longValue(), zeroThreshold,
new ExponentialBucket(getOffset(), getBucketCounts()), EMPTY_EXPONENTIAL_BUCKET);
new ExponentialBuckets(getOffset(), getBucketCounts()), EMPTY_EXPONENTIAL_BUCKET);
}

/**
Expand Down
14 changes: 7 additions & 7 deletions ...c/main/java/io/micrometer/registry/otlp/internal/DefaultExponentialHistogramSnapShot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/
package io.micrometer.registry.otlp.internal;

import static io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBucket.EMPTY_EXPONENTIAL_BUCKET;
import static io.micrometer.registry.otlp.internal.ExponentialHistogramSnapShot.ExponentialBuckets.EMPTY_EXPONENTIAL_BUCKET;

import java.util.LinkedHashMap;
import java.util.Map;
Expand All @@ -37,12 +37,12 @@ protected boolean removeEldestEntry(final Map.Entry eldest) {

private final double zeroThreshold;

private final ExponentialBucket positive;
private final ExponentialBuckets positive;

private final ExponentialBucket negative;
private final ExponentialBuckets negative;

DefaultExponentialHistogramSnapShot(int scale, long zeroCount, double zeroThreshold, ExponentialBucket positive,
ExponentialBucket negative) {
DefaultExponentialHistogramSnapShot(int scale, long zeroCount, double zeroThreshold, ExponentialBuckets positive,
ExponentialBuckets negative) {
this.scale = scale;
this.zeroCount = zeroCount;
this.zeroThreshold = zeroThreshold;
Expand All @@ -61,12 +61,12 @@ public long zeroCount() {
}

@Override
public ExponentialBucket positive() {
public ExponentialBuckets positive() {
return positive;
}

@Override
public ExponentialBucket negative() {
public ExponentialBuckets negative() {
return negative;
}

Expand Down
13 changes: 8 additions & 5 deletions ...otlp/src/main/java/io/micrometer/registry/otlp/internal/ExponentialHistogramSnapShot.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ public interface ExponentialHistogramSnapShot {
/**
* Returns the positive range of exponential bucket counts.
*/
ExponentialBucket positive();
ExponentialBuckets positive();

/**
* Returns the negative range of exponential bucket counts.
*/
ExponentialBucket negative();
ExponentialBuckets negative();

/**
* Returns the threshold below which (inclusive) the values are counted in
Expand All @@ -58,16 +58,19 @@ public interface ExponentialHistogramSnapShot {

boolean isEmpty();

final class ExponentialBucket {
/**
* Represents a dense representation exponential bucket counts.
*/
final class ExponentialBuckets {

public static final ExponentialBucket EMPTY_EXPONENTIAL_BUCKET = new ExponentialBucket(0,
public static final ExponentialBuckets EMPTY_EXPONENTIAL_BUCKET = new ExponentialBuckets(0,
Collections.emptyList());

private final int offset;

private final List<Long> bucketCounts;

ExponentialBucket(int offset, List<Long> bucketCounts) {
ExponentialBuckets(int offset, List<Long> bucketCounts) {
this.offset = offset;
this.bucketCounts = bucketCounts;
}
Expand Down
Loading

0 comments on commit 0108fda

Please sign in to comment.