Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Adapt otel exponential histogram data #10449

Merged
merged 14 commits into from
Feb 28, 2023

Conversation

mufiye
Copy link
Contributor

@mufiye mufiye commented Feb 25, 2023

I find the OpenTelemetryMetricRequestProcessor#adaptMetrics in otel-receiver-plugin can not process exponential histogram metric. But some service will use this exponentialHistogram instead of histogram metric, such as statsd receiver 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 #.

  • Update the CHANGES log.

@wu-sheng
Copy link
Member

Please build a test case for the convert, as otel exponential histogram is pretty new for many people.

@wu-sheng wu-sheng added backend OAP backend related. enhancement Enhancement on performance or codes metrics Metrics and meter labels Feb 25, 2023
@mufiye
Copy link
Contributor Author

mufiye commented Feb 26, 2023

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.

@wu-sheng
Copy link
Member

wu-sheng commented Feb 26, 2023

We just need a unit test. What is in the test folder(root) is e2e testing.
Eventually, we will need that for airflow monitoring, but it doesn't check data correction, mostly just prove data flow.
But this one is very detailed data, so a JUnit based UT is enough.

@wu-sheng wu-sheng requested a review from kezhenxu94 February 27, 2023 03:26
@wu-sheng
Copy link
Member

@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.

@wu-sheng wu-sheng added this to the 9.4.0 milestone Feb 27, 2023
@mufiye
Copy link
Contributor Author

mufiye commented Feb 27, 2023

@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.

@wu-sheng
Copy link
Member

@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.

@wu-sheng
Copy link
Member

Check how SampleFamily#histogram_percentile works(Ref Analyzer#init, createMetric).

@mufiye
Copy link
Contributor Author

mufiye commented Feb 27, 2023

@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.

@wu-sheng
Copy link
Member

@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.

@mufiye
Copy link
Contributor Author

mufiye commented Feb 27, 2023

@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?

@wu-sheng
Copy link
Member

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.

@mufiye
Copy link
Contributor Author

mufiye commented Feb 27, 2023

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?

@wu-sheng
Copy link
Member

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.

@mufiye
Copy link
Contributor Author

mufiye commented Feb 27, 2023

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 if I want to add this function. 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 only that the latter does the data compression.

@wu-sheng
Copy link
Member

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.

…n/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessor.java
wu-sheng
wu-sheng previously approved these changes Feb 27, 2023
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

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

LGTM @kezhenxu94 @wankai123 Please take another look.

@wu-sheng wu-sheng requested a review from kezhenxu94 February 27, 2023 14:38
mufiye and others added 3 commits February 28, 2023 10:44
…t/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java

Co-authored-by: kezhenxu94 <[email protected]>
…t/java/org/apache/skywalking/oap/server/receiver/otel/otlp/OpenTelemetryMetricRequestProcessorTest.java

Co-authored-by: kezhenxu94 <[email protected]>
@wu-sheng wu-sheng requested a review from kezhenxu94 February 28, 2023 03:36
Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

looks good

@wu-sheng wu-sheng merged commit f93a98d into apache:master Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend OAP backend related. enhancement Enhancement on performance or codes metrics Metrics and meter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants