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
receiver/prometheus: add metricGroup.toDistributionPoint pdata conversion #3667
receiver/prometheus: add metricGroup.toDistributionPoint pdata conversion #3667
Conversation
c3f2d8e
to
7eeb9a3
Compare
Kindly cc-ing @tigrannajaryan @bogdandrutu @anuraaga @Aneurysm9 @rakyll. |
662be84
to
f3ad1be
Compare
Implements metricGroupPdata toSummaryPoint and added unit tests as well as equivalence tests to ensure the migration will render the same results. Updates #3137 Depends on PR open-telemetry#3667 Updates PR open-telemetry#3427
Implements metricGroupPdata toSummaryPoint and added unit tests as well as equivalence tests to ensure the migration will render the same results. Updates #3137 Depends on PR open-telemetry#3667 Updates PR open-telemetry#3427
@bogdandrutu @Aneurysm9 @jrcamp @rakyll - please review so that this PR can be merged. It is required part of achieving tracing stability. |
mg.sortPoints() | ||
|
||
// for OCAgent Proto, the bounds won't include +inf | ||
// TODO: (@odeke-em) should we also check OpenTelemetry Pdata for bucket bounds? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The +Inf is implicit in Otel, so no need to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bogdandrutu that code is from the straight up translation from the prior code. To avoid too many moving parts and to keep parity with the existing code, I am going to file an issue to remind me to investigate and finish that TODO that was added ages ago.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed https://github.com/open-telemetry/opentelemetry-collector/issues/3684 and I don't think this change is a blocker for this PR as this PR is a translation to directly use pdata. Thanks for the review and for the answer @bogdandrutu!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, other than the outstanding comment
f3ad1be
to
f67d45d
Compare
Implements metricGroupPdata toSummaryPoint and added unit tests as well as equivalence tests to ensure the migration will render the same results. Updates #3137 Depends on PR open-telemetry#3667 Updates PR open-telemetry#3427
…sion Implements metricGroupPdata toDistributionPoint and added unit tests as well as equivalence tests to ensure the migration will render the same results. Updates #3137 Updates PR open-telemetry#3427
f67d45d
to
b34aa37
Compare
Thanks everyone for the reviews and merge! |
Implements metricGroupPdata toSummaryPoint and added unit tests as well as equivalence tests to ensure the migration will render the same results. Updates #3137 Depends on PR open-telemetry#3667 Updates PR open-telemetry#3427
Implements metricGroupPdata toDistributionPoint and added unit tests
as well as equivalence tests to ensure the migration will render
the same results.
Updates #3137
Updates PR #3427