kezhenxu94 commented on code in PR #10449: URL: https://github.com/apache/skywalking/pull/10449#discussion_r1119433268
########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.oap.server.receiver.otel.otlp; + +import io.opentelemetry.proto.metrics.v1.ExponentialHistogram; +import io.opentelemetry.proto.metrics.v1.ExponentialHistogramDataPoint; +import io.opentelemetry.proto.metrics.v1.Metric; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.skywalking.oap.server.library.module.ModuleManager; +import org.apache.skywalking.oap.server.library.util.prometheus.metrics.Histogram; +import org.apache.skywalking.oap.server.receiver.otel.OtelMetricReceiverConfig; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class OpenTelemetryMetricRequestProcessorTest { + + private OtelMetricReceiverConfig config; + + private ModuleManager manager; + + private OpenTelemetryMetricRequestProcessor metricRequestProcessor; + + private Map<String, String> nodeLabels; + + @BeforeEach + public void setUp() { + manager = new ModuleManager(); + config = new OtelMetricReceiverConfig(); + metricRequestProcessor = new OpenTelemetryMetricRequestProcessor(manager, config); + nodeLabels = new HashMap<>(); + } + + @Test + public void testAdaptExponentialHistogram() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + Class<OpenTelemetryMetricRequestProcessor> clazz = OpenTelemetryMetricRequestProcessor.class; + Method adaptMetricsMethod = clazz.getDeclaredMethod("adaptMetrics", Map.class, Metric.class); + adaptMetricsMethod.setAccessible(true); + + // number is 4, 7, 9 + ExponentialHistogramDataPoint.Buckets var1 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(10) + .addBucketCounts( + 1) // (0, 6.72] + .addBucketCounts( + 1 + ) // (6.72, 8] + .addBucketCounts( + 1 Review Comment: Can you change the bucket counts to different values? For example `1, 2, 3`, so we can verify in the test `1, 2, 3` without worrying we mixed each other and the test still passed ########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.oap.server.receiver.otel.otlp; + +import io.opentelemetry.proto.metrics.v1.ExponentialHistogram; +import io.opentelemetry.proto.metrics.v1.ExponentialHistogramDataPoint; +import io.opentelemetry.proto.metrics.v1.Metric; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.skywalking.oap.server.library.module.ModuleManager; +import org.apache.skywalking.oap.server.library.util.prometheus.metrics.Histogram; +import org.apache.skywalking.oap.server.receiver.otel.OtelMetricReceiverConfig; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class OpenTelemetryMetricRequestProcessorTest { + + private OtelMetricReceiverConfig config; + + private ModuleManager manager; + + private OpenTelemetryMetricRequestProcessor metricRequestProcessor; + + private Map<String, String> nodeLabels; + + @BeforeEach + public void setUp() { + manager = new ModuleManager(); + config = new OtelMetricReceiverConfig(); + metricRequestProcessor = new OpenTelemetryMetricRequestProcessor(manager, config); + nodeLabels = new HashMap<>(); + } + + @Test + public void testAdaptExponentialHistogram() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + Class<OpenTelemetryMetricRequestProcessor> clazz = OpenTelemetryMetricRequestProcessor.class; + Method adaptMetricsMethod = clazz.getDeclaredMethod("adaptMetrics", Map.class, Metric.class); + adaptMetricsMethod.setAccessible(true); + + // number is 4, 7, 9 + ExponentialHistogramDataPoint.Buckets var1 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(10) + .addBucketCounts( + 1) // (0, 6.72] + .addBucketCounts( + 1 + ) // (6.72, 8] + .addBucketCounts( + 1 + ) // (8, 9.51] + .build(); Review Comment: I meant this (I just change the type, please reformat the following lines): ```suggestion var var1 = ExponentialHistogramDataPoint.Buckets.newBuilder() .setOffset(10) .addBucketCounts( 1) // (0, 6.72] .addBucketCounts( 1 ) // (6.72, 8] .addBucketCounts( 1 ) // (8, 9.51] .build(); ``` ########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java: ########## @@ -161,6 +163,57 @@ private static Map<Double, Long> buildBuckets( return result; } + /** + * ExponentialHistogram data points are an alternate representation to the Histogram data point in OpenTelemetry + * metric format(https://opentelemetry.io/docs/reference/specification/metrics/data-model/#exponentialhistogram). + * It uses scale, offset and bucket index to calculate the bound. Firstly, calculate the base using scale by + * formula: base = 2**(2**(-scale)). Then the upperBound of specific bucket can be calculated by formula: + * base**(offset+index+1). Above calculation way is about positive buckets. For the negative case, we just + * map them by their absolute value into the negative range using the same scale as the positive range. So the + * upperBound should be calculated as -base**(offset+index). + * + * Ignored the zero_count field temporarily, + * because the zero_threshold even could overlap the existing bucket scopes. + * + * @param positiveOffset corresponding to positive Buckets' offset in ExponentialHistogramDataPoint + * @param positiveBucketCounts corresponding to positive Buckets' bucket_counts in ExponentialHistogramDataPoint + * @param negativeOffset corresponding to negative Buckets' offset in ExponentialHistogramDataPoint + * @param negativeBucketCounts corresponding to negative Buckets' bucket_counts in ExponentialHistogramDataPoint + * @param scale corresponding to scale in ExponentialHistogramDataPoint + * @return The map is a bucket set for histogram, the key is specific bucket's upperBound, the value is item count + * in this bucket lower than or equals to key(upperBound) + */ + private static Map<Double, Long> buildBucketsFromExponentialHistogram( + int positiveOffset, final List<Long> positiveBucketCounts, + int negativeOffset, final List<Long> negativeBucketCounts, int scale) { + + final Map<Double, Long> result = new HashMap<>(); + double base = Math.pow(2.0, Math.pow(2.0, -scale)); + if (base == Double.POSITIVE_INFINITY) { + log.warn("Receive and reject out-of-range ExponentialHistogram data"); + return result; + } + double upperBound; + for (int i = 0; i < negativeBucketCounts.size(); i++) { + upperBound = -Math.pow(base, negativeOffset + i); + if (upperBound == Double.NEGATIVE_INFINITY) { + log.warn("Receive and reject out-of-range ExponentialHistogram data"); + break; Review Comment: I think you want to `return` here. We don't want to save part of the invalid data ########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java: ########## @@ -161,6 +163,57 @@ private static Map<Double, Long> buildBuckets( return result; } + /** + * ExponentialHistogram data points are an alternate representation to the Histogram data point in OpenTelemetry + * metric format(https://opentelemetry.io/docs/reference/specification/metrics/data-model/#exponentialhistogram). + * It uses scale, offset and bucket index to calculate the bound. Firstly, calculate the base using scale by + * formula: base = 2**(2**(-scale)). Then the upperBound of specific bucket can be calculated by formula: + * base**(offset+index+1). Above calculation way is about positive buckets. For the negative case, we just + * map them by their absolute value into the negative range using the same scale as the positive range. So the + * upperBound should be calculated as -base**(offset+index). + * + * Ignored the zero_count field temporarily, + * because the zero_threshold even could overlap the existing bucket scopes. + * + * @param positiveOffset corresponding to positive Buckets' offset in ExponentialHistogramDataPoint + * @param positiveBucketCounts corresponding to positive Buckets' bucket_counts in ExponentialHistogramDataPoint + * @param negativeOffset corresponding to negative Buckets' offset in ExponentialHistogramDataPoint + * @param negativeBucketCounts corresponding to negative Buckets' bucket_counts in ExponentialHistogramDataPoint + * @param scale corresponding to scale in ExponentialHistogramDataPoint + * @return The map is a bucket set for histogram, the key is specific bucket's upperBound, the value is item count + * in this bucket lower than or equals to key(upperBound) + */ + private static Map<Double, Long> buildBucketsFromExponentialHistogram( + int positiveOffset, final List<Long> positiveBucketCounts, + int negativeOffset, final List<Long> negativeBucketCounts, int scale) { + + final Map<Double, Long> result = new HashMap<>(); + double base = Math.pow(2.0, Math.pow(2.0, -scale)); + if (base == Double.POSITIVE_INFINITY) { + log.warn("Receive and reject out-of-range ExponentialHistogram data"); + return result; + } + double upperBound; + for (int i = 0; i < negativeBucketCounts.size(); i++) { + upperBound = -Math.pow(base, negativeOffset + i); + if (upperBound == Double.NEGATIVE_INFINITY) { + log.warn("Receive and reject out-of-range ExponentialHistogram data"); + break; + } + result.put(upperBound, negativeBucketCounts.get(i)); + } + for (int i = 0; i < positiveBucketCounts.size() - 1; i++) { + upperBound = Math.pow(base, positiveOffset + i + 1); + if (upperBound == Double.POSITIVE_INFINITY) { + log.warn("Receive and reject out-of-range ExponentialHistogram data"); + break; Review Comment: Same here, `return` ########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.oap.server.receiver.otel.otlp; + +import io.opentelemetry.proto.metrics.v1.ExponentialHistogram; +import io.opentelemetry.proto.metrics.v1.ExponentialHistogramDataPoint; +import io.opentelemetry.proto.metrics.v1.Metric; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.skywalking.oap.server.library.module.ModuleManager; +import org.apache.skywalking.oap.server.library.util.prometheus.metrics.Histogram; +import org.apache.skywalking.oap.server.receiver.otel.OtelMetricReceiverConfig; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class OpenTelemetryMetricRequestProcessorTest { + + private OtelMetricReceiverConfig config; + + private ModuleManager manager; + + private OpenTelemetryMetricRequestProcessor metricRequestProcessor; + + private Map<String, String> nodeLabels; + + @BeforeEach + public void setUp() { + manager = new ModuleManager(); + config = new OtelMetricReceiverConfig(); + metricRequestProcessor = new OpenTelemetryMetricRequestProcessor(manager, config); + nodeLabels = new HashMap<>(); + } + + @Test + public void testAdaptExponentialHistogram() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + Class<OpenTelemetryMetricRequestProcessor> clazz = OpenTelemetryMetricRequestProcessor.class; + Method adaptMetricsMethod = clazz.getDeclaredMethod("adaptMetrics", Map.class, Metric.class); + adaptMetricsMethod.setAccessible(true); + + // number is 4, 7, 9 + ExponentialHistogramDataPoint.Buckets var1 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(10) + .addBucketCounts( + 1) // (0, 6.72] + .addBucketCounts( + 1 + ) // (6.72, 8] + .addBucketCounts( + 1 + ) // (8, 9.51] + .build(); + // number is -14, -17, -18, -21 + ExponentialHistogramDataPoint.Buckets var2 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(15) + .addBucketCounts( + 1 + ) // (-16, -13.45] + .addBucketCounts( + 2 + ) // (-19.02, -16] + .addBucketCounts( + 1 + ) // (-INFINITY, -19.02] + .build(); + ExponentialHistogramDataPoint dataPoint = ExponentialHistogramDataPoint.newBuilder() + .setCount(7) + .setSum(-50) + .setScale(2) + .setPositive(var1) + .setNegative(var2) + .setTimeUnixNano(1000000) + .build(); Review Comment: ```suggestion var var2 = ExponentialHistogramDataPoint.Buckets.newBuilder() .setOffset(15) .addBucketCounts( 1 ) // (-16, -13.45] .addBucketCounts( 2 ) // (-19.02, -16] .addBucketCounts( 1 ) // (-INFINITY, -19.02] .build(); var dataPoint = ExponentialHistogramDataPoint.newBuilder() .setCount(7) .setSum(-50) .setScale(2) .setPositive(var1) .setNegative(var2) .setTimeUnixNano(1000000) .build(); ``` ########## oap-server/server-receiver-plugin/otel-receiver-plugin/src/test/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java: ########## @@ -0,0 +1,134 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.skywalking.oap.server.receiver.otel.otlp; + +import io.opentelemetry.proto.metrics.v1.ExponentialHistogram; +import io.opentelemetry.proto.metrics.v1.ExponentialHistogramDataPoint; +import io.opentelemetry.proto.metrics.v1.Metric; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.apache.skywalking.oap.server.library.module.ModuleManager; +import org.apache.skywalking.oap.server.library.util.prometheus.metrics.Histogram; +import org.apache.skywalking.oap.server.receiver.otel.OtelMetricReceiverConfig; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class OpenTelemetryMetricRequestProcessorTest { + + private OtelMetricReceiverConfig config; + + private ModuleManager manager; + + private OpenTelemetryMetricRequestProcessor metricRequestProcessor; + + private Map<String, String> nodeLabels; + + @BeforeEach + public void setUp() { + manager = new ModuleManager(); + config = new OtelMetricReceiverConfig(); + metricRequestProcessor = new OpenTelemetryMetricRequestProcessor(manager, config); + nodeLabels = new HashMap<>(); + } + + @Test + public void testAdaptExponentialHistogram() throws NoSuchMethodException, InvocationTargetException, IllegalAccessException { + Class<OpenTelemetryMetricRequestProcessor> clazz = OpenTelemetryMetricRequestProcessor.class; + Method adaptMetricsMethod = clazz.getDeclaredMethod("adaptMetrics", Map.class, Metric.class); + adaptMetricsMethod.setAccessible(true); + + // number is 4, 7, 9 + ExponentialHistogramDataPoint.Buckets var1 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(10) + .addBucketCounts( + 1) // (0, 6.72] + .addBucketCounts( + 1 + ) // (6.72, 8] + .addBucketCounts( + 1 + ) // (8, 9.51] + .build(); + // number is -14, -17, -18, -21 + ExponentialHistogramDataPoint.Buckets var2 = ExponentialHistogramDataPoint.Buckets.newBuilder() + .setOffset(15) + .addBucketCounts( + 1 + ) // (-16, -13.45] + .addBucketCounts( + 2 + ) // (-19.02, -16] + .addBucketCounts( + 1 Review Comment: Same here, perhaps change to `3` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@skywalking.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
