You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "mufiye (via GitHub)" <gi...@apache.org> on 2023/02/25 15:40:45 UTC
[GitHub] [skywalking] mufiye opened a new pull request, #10449: Adapt otel exponential histogram data
mufiye opened a new pull request, #10449:
URL: https://github.com/apache/skywalking/pull/10449
<!--
⚠️ Please make sure to read this template first, pull requests that don't accord with this template
maybe closed without notice.
Texts surrounded by `<` and `>` are meant to be replaced by you, e.g. <framework name>, <issue number>.
Put an `x` in the `[ ]` to mark the item as CHECKED. `[x]`
-->
<!-- ==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👇 ====
### Fix <bug description or the bug issue number or bug issue link>
- [ ] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
==== 🐛 Remove this line WHEN AND ONLY WHEN you're fixing a bug, follow the checklist 👆 ==== -->
<!-- ==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👇 ====
### Improve the performance of <class or module or ...>
- [ ] Add a benchmark for the improvement, refer to [the existing ones](https://github.com/apache/skywalking/blob/master/apm-commons/apm-datacarrier/src/test/java/org/apache/skywalking/apm/commons/datacarrier/LinkedArrayBenchmark.java)
- [ ] The benchmark result.
```text
<Paste the benchmark results here>
```
- [ ] Links/URLs to the theory proof or discussion articles/blogs. <links/URLs here>
==== 📈 Remove this line WHEN AND ONLY WHEN you're improving the performance, follow the checklist 👆 ==== -->
### <Feature description>
I find the `OpenTelemetryMetricRequestProcessor#adaptMetrics` in otel-receiver-plugin can not process [exponential histogram metric](https://opentelemetry.io/docs/reference/specification/metrics/data-model/#exponentialhistogram). But some service will use this exponentialHistogram instead of histogram metric, such as [statsd receiver](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/receiver/statsdreceiver/README.md) in opentelemetry collector. So I do this pull request to adapt this case.
The related discussion about this pull request is in [issue #10341].
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng merged pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #10449:
URL: https://github.com/apache/skywalking/pull/10449
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118289057
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
From my understanding, zero means less than a threshold, no matter this is less than which boundaries.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446246228
> > @mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double? Because when we calculate percentile from the histogram (usually we do today), the values are bucket boundaries and they should be integer/long only.
>
> I have understood this question. I think the small double case maybe happen. But this case also maybe happen for otel histogram data.
If current, the buckets of airflow definition are good, we should be fine. Otherwise, we may need some closure to process the buckets, such as maybe, we need multiple 1000 from microsecond to millisecond precision.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118842586
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> The precise value for the zero_threshold is arbitrary and not related to the scale.
**not related to the scale** means **not related to the bucket**. The buckets are made by (2**2**-scale)
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118496823
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> We could leave some comments here as a TBD about ignoring this, maybe one day, we will know how should we process this. WDYT?
I'm back from class. I read the docs and the new opentelemetry proto file. I think it is a nice idea. Anyway for the airflow data, I have not received zero_count data.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446259412
> > > @mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double? Because when we calculate percentile from the histogram (usually we do today), the values are bucket boundaries and they should be integer/long only.
> >
> >
> > I have understood this question. I think the small double case maybe happen. But this case also maybe happen for otel histogram data.
>
> If current, the buckets of airflow definition are good, we should be fine. Otherwise, we may need some closure to process the buckets, such as maybe, we need multiple 1000 from microsecond to millisecond precision.
I review the data collected from airflow and find there are some small timer data, such as 1.182709 and 1.348083 in one exponentialHistogram data. Where should I add the closure?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118374760
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> We could leave some comments here as a TBD about ignoring this, maybe one day, we will know how should we process this. WDYT?
Good to me
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446371235
> > > I review the data collected from airflow and find there are some small timer data, such as 1.182709 and 1.348083 in one exponentialHistogram data. Where should I add the closure?
> >
> >
> > When you write MAL.
>
> I read other services' otel rule cases and meter analysis language but have no idea how to do this. Could you explain it more?
There is no other for these. Because they don't use small doubles as buckets.
If you want to learn this for now, then MAL is groovy based script. `SampleFamily` is the root class running in MAL engine, and this is the type referred by the metric name of the expression.
You can see `SampleFamily#filter(Closure<Boolean> filter)`, this is what I mean about closure. It means a code block as an inner class for the metric process.
We don't have a similar mechanism for this, but from what you described these buckets, we seem to need one for the exponential histogram. For example, we may need `buildBucketsFromExponentialHistogram(..., Closure<Boolean> processor)` as another new method. Then we could write some codes in the MAL expression to do `mutiple 1000`(ns -> ms) mentioned above.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446398092
> If you want to learn this for now, then MAL is groovy based script. `SampleFamily` is the root class running in MAL engine, and this is the type referred by the metric name of the expression. You can see `SampleFamily#filter(Closure<Boolean> filter)`, this is what I mean about closure. It means a code block as an inner class for the metric process. We don't have a similar mechanism for this, but from what you described these buckets, we seem to need one for the exponential histogram. For example, we may need `buildBucketsFromExponentialHistogram(..., Closure<Boolean> processor)` as another new method. Then we could write some codes in the MAL expression to do `mutiple 1000`(ns -> ms) mentioned above.
I think I need to fully understand the meter analyzer code first. In my current opinion, the function can not be put into the `OpenTelemetryMetricRequestProcessor#adaptMetrics` part and should be put in the analyzer part. And should we also consider small double data case for "otel histogram data"? Anyway, the difference between histogram and ExponentialHistogram is that the latter does the data compression.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118088391
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -187,7 +187,7 @@ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
final Map<Double, Long> result = new HashMap<>();
double base = Math.pow(2.0, Math.pow(2.0, -scale));
if (base == Double.POSITIVE_INFINITY) {
- if (log.isDebugEnabled()) {
+ if (log.isWarnEnabled()) {
log.warn("Receive and reject out-of-range ExponentialHistogram data");
}
Review Comment:
You don't have extra operation besides the logout, then usually, with or w/o `isWarnEnabled`, they are nearly same.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446353848
> > I review the data collected from airflow and find there are some small timer data, such as 1.182709 and 1.348083 in one exponentialHistogram data. Where should I add the closure?
>
> When you write MAL.
I read other services' otel rule cases and meter analysis language but have no idea how to do this. Could you explain it more?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118287651
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
I think I miss this part, I would like to assign its upper bound as the lower bound of small positive, WDYT?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445260453
> Please build a test case for the convert, as `otel exponential histogram` is pretty new for many people.
Ok, I get it. I will add the test case into the test dir. I think I need some time to learn how to build my test case because it is different from the plugin-test which I done before.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1117940792
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,24 @@ private static Map<Double, Long> buildBuckets(
return result;
}
+ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
Review Comment:
Please add detailed and well formatted comments about the converting logic. It is new.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118296408
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> There is clear definition of zeros in OTEL data model, we can tackle the zeros according to the definition, in short, the zero_count is the count of bucket (-threshold, +threshold), or count of samples that are approximately 0 if threshold is not set.
Yes, they defined. Last time, I check, it mentioned, the zero threshold even could cover over the existing bucket scopes.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118292694
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> I think I miss this part, I would like to assign its upper bound as the lower bound of small positive, WDYT?
> From my understanding, zero means less than a threshold, no matter this is less than which boundaries.
There is clear definition of zeros in OTEL data model, we can tackle the zeros according to the definition, in short, the `zero_count` is the count of bucket `(-threshold, +threshold)`, or count of samples that are approximately `0` if `threshold` is not set.
> The ExponentialHistogram contains a special zero_count bucket and an optional zero_threshold field where zero_count contains the count of values whose absolute value is less than or equal to zero_threshold. The precise value for the zero_threshold is arbitrary and not related to the scale.
> When zero_threshold is unset or 0, this bucket stores values that cannot be expressed using the standard exponential formula as well as values that have been rounded to zero.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118021887
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,24 @@ private static Map<Double, Long> buildBuckets(
return result;
}
+ 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));
+ double upperBound;
+ for (int i = 0; i < negativeBucketCounts.size(); i++) {
+ upperBound = -Math.pow(base, negativeOffset + i);
+ result.put(upperBound, negativeBucketCounts.get(i));
+ }
+ for (int i = 0; i < positiveBucketCounts.size(); i++) {
+ upperBound = Math.pow(base, positiveOffset + i + 1);
+ result.put(upperBound, positiveBucketCounts.get(i));
+ }
Review Comment:
Ok, I will add it.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118088809
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -187,7 +187,7 @@ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
final Map<Double, Long> result = new HashMap<>();
double base = Math.pow(2.0, Math.pow(2.0, -scale));
if (base == Double.POSITIVE_INFINITY) {
- if (log.isDebugEnabled()) {
+ if (log.isWarnEnabled()) {
log.warn("Receive and reject out-of-range ExponentialHistogram data");
}
Review Comment:
> You don't have extra operation besides the logout, then usually, with or w/o `isWarnEnabled`, they are nearly same.
So should I delete the if judge statement? And for the error, I think it is because I use jdk since 16. I will try to use jdk 11.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445645749
@mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double? Because when we calculate percentile from the histogram (usually we do today), the values are bucket boundaries and they should be integer/long only.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445782537
Check how `SampleFamily#histogram_percentile` works(Ref Analyzer#init, createMetric).
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118090759
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -187,7 +187,7 @@ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
final Map<Double, Long> result = new HashMap<>();
double base = Math.pow(2.0, Math.pow(2.0, -scale));
if (base == Double.POSITIVE_INFINITY) {
- if (log.isDebugEnabled()) {
+ if (log.isWarnEnabled()) {
log.warn("Receive and reject out-of-range ExponentialHistogram data");
}
Review Comment:
> JDK 11 is our minimum JDK requirement. You could set IDE(IntelliJ Language level) to JDK 11 for code APIs. But JDK16 is not an LTS version, JDK 11 and 17 are a better choice.
Ok, I get it. I use the JDK 17 before.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118496823
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> We could leave some comments here as a TBD about ignoring this, maybe one day, we will know how should we process this. WDYT?
I'm back from class. I read the docs and the new opentelemetry proto file. I think it is a nice idea.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446261363
> I review the data collected from airflow and find there are some small timer data, such as 1.182709 and 1.348083 in one exponentialHistogram data. Where should I add the closure?
When you write MAL.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118833407
##########
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).
+ *
+ * We temporarily ignored the zero_count field, because the zero_threshold even could cover over the existing
+ * bucket scopes. The discussion is in pull request #10449.
Review Comment:
```suggestion
* Ignored the zero_count field temporarily,
* because the zero_threshold even could overlap the existing bucket scopes.
```
Usually, we don't use `we` in the comments. The codes are we :)
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1117997973
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,24 @@ private static Map<Double, Long> buildBuckets(
return result;
}
+ 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));
Review Comment:
Please check all power calculations whether they are overflowed and reject the data if so.
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,24 @@ private static Map<Double, Long> buildBuckets(
return result;
}
+ 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));
+ double upperBound;
+ for (int i = 0; i < negativeBucketCounts.size(); i++) {
+ upperBound = -Math.pow(base, negativeOffset + i);
+ result.put(upperBound, negativeBucketCounts.get(i));
+ }
+ for (int i = 0; i < positiveBucketCounts.size(); i++) {
+ upperBound = Math.pow(base, positiveOffset + i + 1);
+ result.put(upperBound, positiveBucketCounts.get(i));
+ }
Review Comment:
Looks like there missed a upper bound for +Infitite
```suggestion
for (int i = 0; i < positiveBucketCounts.size() - 1; i++) {
upperBound = Math.pow(base, positiveOffset + i + 1);
result.put(upperBound, positiveBucketCounts.get(i));
}
result.put(Double.POSITIVE_INFINITY, positiveBucketCounts.get(positiveBucketCounts.size() - 1));
```
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118252273
##########
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 positiveBuckets = ExponentialHistogramDataPoint.Buckets.newBuilder()
Review Comment:
The type is a bit long, causing the entire line to be very long, consider using `var` so the lines can be shorten
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
Looks like you didn't take into consideration of the `zero_count` field in OTEL exponential histgram data model? Can you elaborate why?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446202639
> @mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double? Because when we calculate percentile from the histogram (usually we do today), the values are bucket boundaries and they should be integer/long only.
I have understood this question. I think the small double case maybe happen.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1446440510
> the function can not be put into the OpenTelemetryMetricRequestProcessor#adaptMetrics part and should be put in the analyzer part. And should we also consider small double data case for "otel histogram data"?
Yes, it should be on the MAL(SampleFamily) part.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118668885
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
Review Comment:
@mufiye You could add some comments here about `zero_count`.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118651899
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
About comments, should I add them to code?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118090452
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -187,7 +187,7 @@ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
final Map<Double, Long> result = new HashMap<>();
double base = Math.pow(2.0, Math.pow(2.0, -scale));
if (base == Double.POSITIVE_INFINITY) {
- if (log.isDebugEnabled()) {
+ if (log.isWarnEnabled()) {
log.warn("Receive and reject out-of-range ExponentialHistogram data");
}
Review Comment:
JDK 11 is our minimum JDK requirement. You could set IDE(IntelliJ Language level) to JDK 11 for code APIs.
But JDK16 is not an LTS version, JDK 11 and 17 are a better choice.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118293848
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
Ignoring the count make sense when do percentile, as it should not be considered as base.
But if we read the count, maybe this helps.
This is just my understanding.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445719573
> > @mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double?
>
>
>
> This histogram always builds bucket boundaries as double. I don't fully understand what you mean.
>
>
Check hisgram percentile function, which calculate the value. The percentile value is the rank of the bucket.
Read the calculation will help you to understand the process.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118334799
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
We could leave some comments here as a TBD about ignoring this, maybe one day, we will know how should we process this. WDYT?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118090513
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -187,7 +187,7 @@ private static Map<Double, Long> buildBucketsFromExponentialHistogram(
final Map<Double, Long> result = new HashMap<>();
double base = Math.pow(2.0, Math.pow(2.0, -scale));
if (base == Double.POSITIVE_INFINITY) {
- if (log.isDebugEnabled()) {
+ if (log.isWarnEnabled()) {
log.warn("Receive and reject out-of-range ExponentialHistogram data");
}
Review Comment:
Unless you built more complex logic in the logging, you don't need this for every debug.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445265630
We just need a unit test.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445145271
Please build a test case for the convert, as `otel exponential histogram` is pretty new for many people.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#issuecomment-1445699694
> @mufiye One question, does this histogram always build bucket boundaries as integers? Could it be a very small double?
This histogram always builds bucket boundaries as double. I don't fully understand what you mean.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118332343
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
Check this https://opentelemetry.io/docs/reference/specification/metrics/data-model/#exponentialhistogram
> The ExponentialHistogram contains a special zero_count bucket and an optional zero_threshold field where zero_count contains the count of values whose absolute value is less than or equal to zero_threshold. The precise value for the zero_threshold is arbitrary and not related to the scale.
So, we can't assume the range of the threshold would not overlap with existing buckets. We can't build more buckets from it.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] mufiye commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "mufiye (via GitHub)" <gi...@apache.org>.
mufiye commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118021996
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,24 @@ private static Map<Double, Long> buildBuckets(
return result;
}
+ 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));
Review Comment:
> Please check all power calculations whether they are overflowed and reject the data if so.
Ok, I will add the check.
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10449: Adapt otel exponential histogram data
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10449:
URL: https://github.com/apache/skywalking/pull/10449#discussion_r1118837536
##########
oap-server/server-receiver-plugin/otel-receiver-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java:
##########
@@ -161,6 +163,54 @@ 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).
+ *
+ * @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<>();
Review Comment:
> Check this https://opentelemetry.io/docs/reference/specification/metrics/data-model/#exponentialhistogram
>
>
>
> > The ExponentialHistogram contains a special zero_count bucket and an optional zero_threshold field where zero_count contains the count of values whose absolute value is less than or equal to zero_threshold. The precise value for the zero_threshold is arbitrary and not related to the scale.
>
>
>
> So, we can't assume the range of the threshold would not overlap with existing buckets. We can't build more buckets from it.
Actually I am curious where does it indicate the threshold can overlap with existing buckets ?
--
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-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org