Skip to content

Commit 91a480d

Browse files
felixbarnyfelixbarny
committed
Address comments from review
1 parent 8302c2c commit 91a480d

File tree

3 files changed

+86
-52
lines changed

3 files changed

+86
-52
lines changed

‎x-pack/plugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/DataPoint.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -233,12 +233,12 @@ public String getDynamicTemplate(MappingHints mappingHints) {
233233

234234
@Override
235235
public boolean isValid(Set<String> messages) {
236-
boolean valid = metric.getExponentialHistogram()
237-
.getAggregationTemporality() == AggregationTemporality.AGGREGATION_TEMPORALITY_DELTA;
238-
if (valid == false) {
236+
if (metric.getExponentialHistogram()
237+
.getAggregationTemporality() == AggregationTemporality.AGGREGATION_TEMPORALITY_DELTA == false) {
239238
messages.add("cumulative exponential histogram metrics are not supported, ignoring " + metric.getName());
239+
return false;
240240
}
241-
return valid;
241+
return true;
242242
}
243243
}
244244

@@ -303,11 +303,15 @@ public String getDynamicTemplate(MappingHints mappingHints) {
303303

304304
@Override
305305
public boolean isValid(Set<String> messages) {
306-
boolean valid = metric.getHistogram().getAggregationTemporality() == AggregationTemporality.AGGREGATION_TEMPORALITY_DELTA;
307-
if (valid == false) {
306+
if (metric.getHistogram().getAggregationTemporality() == AggregationTemporality.AGGREGATION_TEMPORALITY_DELTA == false) {
308307
messages.add("cumulative histogram metrics are not supported, ignoring " + metric.getName());
308+
return false;
309+
}
310+
if (dataPoint.getBucketCountsCount() == 1 && dataPoint.getExplicitBoundsCount() == 0) {
311+
messages.add("histogram with a single bucket and no explicit bounds is not supported, ignoring " + metric.getName());
312+
return false;
309313
}
310-
return valid;
314+
return true;
311315
}
312316
}
313317

‎x-pack/plugin/otel-data/src/main/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/HistogramConverter.java‎

Lines changed: 30 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,9 @@ static <E extends Exception> void counts(ExponentialHistogramDataPoint dp, Check
2929

3030
for (int i = negative.getBucketCountsCount() - 1; i >= 0; i--) {
3131
long count = negative.getBucketCounts(i);
32-
if (count == 0) {
33-
continue;
32+
if (count != 0) {
33+
counts.accept(count);
3434
}
35-
counts.accept(count);
3635
}
3736

3837
long zeroCount = dp.getZeroCount();
@@ -43,10 +42,9 @@ static <E extends Exception> void counts(ExponentialHistogramDataPoint dp, Check
4342
ExponentialHistogramDataPoint.Buckets positive = dp.getPositive();
4443
for (int i = 0; i < positive.getBucketCountsCount(); i++) {
4544
long count = positive.getBucketCounts(i);
46-
if (count == 0) {
47-
continue;
45+
if (count != 0) {
46+
counts.accept(count);
4847
}
49-
counts.accept(count);
5048
}
5149
}
5250

@@ -63,12 +61,11 @@ static <E extends Exception> void centroidValues(ExponentialHistogramDataPoint d
6361
int offset = negative.getOffset();
6462
for (int i = negative.getBucketCountsCount() - 1; i >= 0; i--) {
6563
long count = negative.getBucketCounts(i);
66-
if (count == 0) {
67-
continue;
64+
if (count != 0) {
65+
double lb = -ExponentialScaleUtils.getUpperBucketBoundary(offset + i, scale);
66+
double ub = -ExponentialScaleUtils.getLowerBucketBoundary(offset + i, scale);
67+
values.accept(lb + (ub - lb) / 2);
6868
}
69-
double lb = -ExponentialScaleUtils.getLowerBucketBoundary(offset + i, scale);
70-
double ub = -ExponentialScaleUtils.getUpperBucketBoundary(offset + i, scale);
71-
values.accept(lb + (ub - lb) / 2);
7269
}
7370

7471
long zeroCount = dp.getZeroCount();
@@ -80,22 +77,20 @@ static <E extends Exception> void centroidValues(ExponentialHistogramDataPoint d
8077
offset = positive.getOffset();
8178
for (int i = 0; i < positive.getBucketCountsCount(); i++) {
8279
long count = positive.getBucketCounts(i);
83-
if (count == 0) {
84-
continue;
80+
if (count != 0) {
81+
double lb = ExponentialScaleUtils.getLowerBucketBoundary(offset + i, scale);
82+
double ub = ExponentialScaleUtils.getUpperBucketBoundary(offset + i, scale);
83+
values.accept(lb + (ub - lb) / 2);
8584
}
86-
double lb = ExponentialScaleUtils.getLowerBucketBoundary(offset + i, scale);
87-
double ub = ExponentialScaleUtils.getUpperBucketBoundary(offset + i, scale);
88-
values.accept(lb + (ub - lb) / 2);
8985
}
9086
}
9187

9288
static <E extends Exception> void counts(HistogramDataPoint dp, CheckedLongConsumer<E> counts) throws E {
9389
for (int i = 0; i < dp.getBucketCountsCount(); i++) {
9490
long count = dp.getBucketCounts(i);
95-
if (count == 0) {
96-
continue;
91+
if (count != 0) {
92+
counts.accept(count);
9793
}
98-
counts.accept(count);
9994
}
10095
}
10196

@@ -109,25 +104,24 @@ static <E extends Exception> void centroidValues(HistogramDataPoint dp, CheckedD
109104
int size = dp.getBucketCountsCount();
110105
for (int i = 0; i < size; i++) {
111106
long count = dp.getBucketCounts(i);
112-
if (count == 0) {
113-
continue;
114-
}
115-
double value;
116-
if (i == 0) {
117-
// (-infinity, explicit_bounds[i]]
118-
value = dp.getExplicitBounds(i);
119-
if (value > 0) {
120-
value /= 2;
107+
if (count != 0) {
108+
double value;
109+
if (i == 0) {
110+
// (-infinity, explicit_bounds[i]]
111+
value = dp.getExplicitBounds(i);
112+
if (value > 0) {
113+
value /= 2;
114+
}
115+
} else if (i == size - 1) {
116+
// (explicit_bounds[i], +infinity)
117+
value = dp.getExplicitBounds(i - 1);
118+
} else {
119+
// [explicit_bounds[i-1], explicit_bounds[i])
120+
// Use the midpoint between the boundaries.
121+
value = dp.getExplicitBounds(i - 1) + (dp.getExplicitBounds(i) - dp.getExplicitBounds(i - 1)) / 2.0;
121122
}
122-
} else if (i == size - 1) {
123-
// (explicit_bounds[i], +infinity)
124-
value = dp.getExplicitBounds(i - 1);
125-
} else {
126-
// [explicit_bounds[i-1], explicit_bounds[i])
127-
// Use the midpoint between the boundaries.
128-
value = dp.getExplicitBounds(i - 1) + (dp.getExplicitBounds(i) - dp.getExplicitBounds(i - 1)) / 2.0;
123+
values.accept(value);
129124
}
130-
values.accept(value);
131125
}
132126
}
133127

‎x-pack/plugin/otel-data/src/test/java/org/elasticsearch/xpack/oteldata/otlp/datapoint/HistogramConverterTests.java‎

Lines changed: 45 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,31 +7,59 @@
77

88
package org.elasticsearch.xpack.oteldata.otlp.datapoint;
99

10+
import io.opentelemetry.proto.metrics.v1.AggregationTemporality;
11+
import io.opentelemetry.proto.metrics.v1.Histogram;
1012
import io.opentelemetry.proto.metrics.v1.HistogramDataPoint;
13+
import io.opentelemetry.proto.metrics.v1.Metric;
1114

1215
import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
1316

1417
import org.elasticsearch.test.ESTestCase;
1518

1619
import java.util.ArrayList;
20+
import java.util.HashSet;
1721
import java.util.List;
1822

23+
import static org.hamcrest.Matchers.equalTo;
24+
1925
public class HistogramConverterTests extends ESTestCase {
2026

2127
@SuppressWarnings("unused")
2228
private final String name;
2329
private final HistogramDataPoint dataPoint;
2430
private final List<Long> expectedCounts;
2531
private final List<Double> expectedValues;
32+
private final boolean valid;
2633

27-
public HistogramConverterTests(String name, HistogramDataPoint dataPoint, List<Long> expectedCounts, List<Double> expectedValues) {
34+
public HistogramConverterTests(
35+
String name,
36+
HistogramDataPoint dataPoint,
37+
List<Long> expectedCounts,
38+
List<Double> expectedValues,
39+
boolean valid
40+
) {
2841
this.name = name;
2942
this.dataPoint = dataPoint;
3043
this.expectedCounts = expectedCounts;
3144
this.expectedValues = expectedValues;
45+
this.valid = valid;
3246
}
3347

3448
public void testHistograms() throws Exception {
49+
DataPoint.Histogram histogram = new DataPoint.Histogram(
50+
dataPoint,
51+
Metric.newBuilder()
52+
.setHistogram(
53+
Histogram.newBuilder()
54+
.setAggregationTemporality(AggregationTemporality.AGGREGATION_TEMPORALITY_DELTA)
55+
.addDataPoints(dataPoint)
56+
)
57+
.build()
58+
);
59+
assertThat(histogram.isValid(new HashSet<>()), equalTo(valid));
60+
if (valid == false) {
61+
return;
62+
}
3563
List<Long> actualCounts = new ArrayList<>();
3664
HistogramConverter.counts(dataPoint, actualCounts::add);
3765
List<Double> actualValues = new ArrayList<>();
@@ -47,48 +75,56 @@ public void testHistograms() throws Exception {
4775
@ParametersFactory(argumentFormatting = "%1$s")
4876
public static List<Object[]> testCases() {
4977
return List.of(
50-
new Object[] { "empty", HistogramDataPoint.newBuilder().build(), List.of(), List.of() },
78+
new Object[] { "empty", HistogramDataPoint.newBuilder().build(), List.of(), List.of(), true },
5179
new Object[] {
5280
"single bucket",
5381
HistogramDataPoint.newBuilder().addBucketCounts(10L).addExplicitBounds(5.0).build(),
5482
List.of(10L),
55-
List.of(2.5) },
83+
List.of(2.5),
84+
true },
85+
new Object[] { "single count", HistogramDataPoint.newBuilder().addBucketCounts(10L).build(), List.of(10L), List.of(), false },
5686
new Object[] {
5787
"two buckets",
5888
HistogramDataPoint.newBuilder().addAllBucketCounts(List.of(5L, 10L)).addExplicitBounds(5.0).build(),
5989
List.of(5L, 10L),
60-
List.of(2.5, 5.0) },
90+
List.of(2.5, 5.0),
91+
true },
6192
new Object[] {
6293
"three buckets",
6394
HistogramDataPoint.newBuilder().addAllBucketCounts(List.of(5L, 10L, 15L)).addAllExplicitBounds(List.of(5.0, 10.0)).build(),
6495
List.of(5L, 10L, 15L),
65-
List.of(2.5, 7.5, 10.0) },
96+
List.of(2.5, 7.5, 10.0),
97+
true },
6698
new Object[] {
6799
"zero count buckets",
68100
HistogramDataPoint.newBuilder().addAllBucketCounts(List.of(5L, 0L, 15L)).addAllExplicitBounds(List.of(5.0, 10.0)).build(),
69101
List.of(5L, 15L),
70-
List.of(2.5, 10.0) },
102+
List.of(2.5, 10.0),
103+
true },
71104
new Object[] {
72105
"negative bounds",
73106
HistogramDataPoint.newBuilder()
74107
.addAllBucketCounts(List.of(5L, 10L, 15L))
75108
.addAllExplicitBounds(List.of(-10.0, 10.0))
76109
.build(),
77110
List.of(5L, 10L, 15L),
78-
List.of(-10.0, 0.0, 10.0) },
111+
List.of(-10.0, 0.0, 10.0),
112+
true },
79113
new Object[] {
80114
"all negative bounds",
81115
HistogramDataPoint.newBuilder().addAllBucketCounts(List.of(5L, 10L)).addExplicitBounds(-5.0).build(),
82116
List.of(5L, 10L),
83-
List.of(-5.0, -5.0) },
117+
List.of(-5.0, -5.0),
118+
true },
84119
new Object[] {
85120
"multiple buckets with varying distances",
86121
HistogramDataPoint.newBuilder()
87122
.addAllBucketCounts(List.of(5L, 10L, 15L, 20L))
88123
.addAllExplicitBounds(List.of(1.0, 5.0, 20.0))
89124
.build(),
90125
List.of(5L, 10L, 15L, 20L),
91-
List.of(0.5, 3.0, 12.5, 20.0) }
126+
List.of(0.5, 3.0, 12.5, 20.0),
127+
true }
92128
);
93129
}
94130
}

0 commit comments

Comments
 (0)