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

receiver/prometheus: add metricGroup.toSummaryPoint pdata conversion #3668

Conversation

odeke-em
Copy link
Member

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 #3667
Updates PR #3427

@odeke-em odeke-em requested review from alolita and a team as code owners July 18, 2021 07:27
@odeke-em odeke-em requested a review from owais July 18, 2021 07:27
@project-bot project-bot bot added this to In progress in Collector Jul 18, 2021
@odeke-em
Copy link
Member Author

Another one, kindly cc-ing @bogdandrutu @Aneurysm9 @rakyll @alolita @tigrannajaryan @anuraaga

odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 19, 2021
Implements metricGroupPdata toNumberDataPoint 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#3668
Updates PR open-telemetry#3427
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 19, 2021
Implements metricGroupPdata toNumberDataPoint 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#3668
Updates PR open-telemetry#3427
@alolita
Copy link
Member

alolita commented Jul 19, 2021

@Aneurysm9 @dashpole @rakyll please review - ty!

Copy link
Contributor

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

toSummaryPoint looks good.

@punya punya added the ready-to-merge Code review completed; ready to merge by maintainers label Jul 20, 2021
@bogdandrutu
Copy link
Member

@dashpole indeed the Summary looks good but depends on a PR with some comments :)

@dashpole
Copy link
Contributor

Yes. This is not ready to merge.

@bogdandrutu bogdandrutu removed the ready-to-merge Code review completed; ready to merge by maintainers label Jul 20, 2021
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 21, 2021
Implements metricGroupPdata toNumberDataPoint 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#3668
Updates PR open-telemetry#3427
@odeke-em odeke-em force-pushed the receiver-prometheus-metricGroupToSummaryPointPdata-migration branch from 8444434 to a601910 Compare July 21, 2021 03:36
@bogdandrutu
Copy link
Member

Please rebase

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
@odeke-em odeke-em force-pushed the receiver-prometheus-metricGroupToSummaryPointPdata-migration branch from a601910 to cb9bd45 Compare July 21, 2021 15:53
@odeke-em
Copy link
Member Author

Please rebase

Rebase done, thanks @bogdandrutu!

@bogdandrutu bogdandrutu merged commit 866143b into open-telemetry:main Jul 21, 2021
Collector automation moved this from In progress to Done Jul 21, 2021
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 21, 2021
Implements metricGroupPdata toNumberDataPoint 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#3668
Updates PR open-telemetry#3427
@odeke-em odeke-em deleted the receiver-prometheus-metricGroupToSummaryPointPdata-migration branch July 21, 2021 17:53
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 21, 2021
Implements metricGroupPdata toNumberDataPoint and added unit tests
as well as equivalence tests to ensure the migration will render
the same results.

While here, added TODOs for issue open-telemetry#3691 which found a bug in
which cumulative types weren't using the actual duration start
timestamp. Given that this current change is a translation of prior
logic and has parity checks, making that bug fix would complicate
the PR.

Updates #3137
Depends on PR open-telemetry#3668
Updates PR open-telemetry#3427
Updates open-telemetry#3691
odeke-em added a commit to orijtech/opentelemetry-collector that referenced this pull request Jul 22, 2021
Implements metricGroupPdata toNumberDataPoint and added unit tests
as well as equivalence tests to ensure the migration will render
the same results.

While here, added TODOs for issue open-telemetry#3691 which found a bug in
which cumulative types weren't using the actual duration start
timestamp. Given that this current change is a translation of prior
logic and has parity checks, making that bug fix would complicate
the PR.

Updates #3137
Depends on PR open-telemetry#3668
Updates PR open-telemetry#3427
Updates open-telemetry#3691
bogdandrutu pushed a commit that referenced this pull request Jul 22, 2021
…on (#3674)

Implements metricGroupPdata toNumberDataPoint and added unit tests
as well as equivalence tests to ensure the migration will render
the same results.

While here, added TODOs for issue #3691 which found a bug in
which cumulative types weren't using the actual duration start
timestamp. Given that this current change is a translation of prior
logic and has parity checks, making that bug fix would complicate
the PR.

Updates #3137
Depends on PR #3668
Updates PR #3427
Updates #3691
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Collector
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants