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.

Sign up for GitHub

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

Jump to bottom

receiver/prometheus/internal: direct metricfamily Prometheus->OTLP + equivalent tests #3145

Merged
merged 11 commits into from
Jun 8, 2021
Merged

receiver/prometheus/internal: direct metricfamily Prometheus->OTLP + equivalent tests #3145

merged 11 commits into from
Jun 8, 2021

Conversation

odeke-em
Copy link
Member

@odeke-em odeke-em commented May 12, 2021 •
edited
Loading

Adds direct metricfamily Prometheus->OTLP conversion as well as equivalence tests.
After this change, there will be 2 other changes coming up and then finally we'll be able
to remove the dependency of OpenCensus in the Prometheus receiver.

Updates #3137
Follow-up of PR #3139 unfortunately given that Github doesn't support chaining PRs.

@odeke-em odeke-em requested a review from a team May 12, 2021 08:57
dashpole
dashpole approved these changes May 19, 2021
View reviewed changes
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.

lgtm with nits

receiver/prometheusreceiver/internal/otlp_metricfamily.go Outdated Show resolved Hide resolved
receiver/prometheusreceiver/internal/otlp_metricfamily.go Show resolved Hide resolved
bogdandrutu
bogdandrutu reviewed May 19, 2021
View reviewed changes
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

@rakyll @Aneurysm9 @dashpole as Prometheus experts please review.

receiver/prometheusreceiver/internal/otlp_metricsbuilder_test.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label May 27, 2021
@odeke-em odeke-em force-pushed the receiver-prometheus-pdataMetricFamily branch from a2c6148 to f0db09e Compare May 27, 2021 14:05
@odeke-em odeke-em requested a review from alolita as a code owner May 27, 2021 14:05
@odeke-em odeke-em requested a review from bogdandrutu May 27, 2021 16:19
@odeke-em
Copy link
Member Author

@bogdandrutu @dashpole please take a look again, unit tests added, but please remember that because Github can't stack PRs this change is waiting on PR #3139.

@odeke-em odeke-em force-pushed the receiver-prometheus-pdataMetricFamily branch from ca20c70 to 4fea08a Compare May 27, 2021 18:11
odeke-em added 8 commits May 27, 2021 21:37
@odeke-em
receiver/prometheus/internal: add createNodeAndResourcePdata for Prom…
19c992f 
…etheus->OTLP Pdata

Starts the progressive effort to convert directly from
    Prometheus->OTLPPdata

directly instead of

    Prometheus->OpenCensusProto->OTLPPdata

by adding a converter for node+resource -> pdata.Resource.

Updates #3137
@odeke-em
Use .Attributes to skip unsafe extraction plus use require.Equal
91c5bbb 
Addresses feedback pointed out by Bogdan, Tigran, Jaana
by using Resource.Attributes().Sort() instead and also avoid
using go-cmp/cmp and instead opt to using require.Equal.
@odeke-em
Add comment and rename parity test check
1c10d8c
@odeke-em
Add createNodeAndResource unit test
d0d2fb6
@odeke-em
receiver/prometheus/internal: direct metricfamily Prometheus->OTLP + …
ae55b79 
…equivalent tests

Updates #3137
@odeke-em
receiver/prometheus: add metricFamily.isCumulative + equivalence tests
74c8edd
@odeke-em
Add unit tests and comments about composition
d40d777
@odeke-em
Address lint warnings
6f38bbc
@odeke-em odeke-em force-pushed the receiver-prometheus-pdataMetricFamily branch from 4fea08a to 6f38bbc Compare May 27, 2021 19:13
@github-actions github-actions bot removed the Stale label May 28, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jun 4, 2021

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jun 4, 2021
@odeke-em
Copy link
Member Author

odeke-em commented Jun 4, 2021

@bogdandrutu, Anurag and David approved the PR, I addressed all the requested changes 7+ days ago; kindly waiting for your review/merge otherwise the PR has now been marked as stale.

@github-actions github-actions bot removed the Stale label Jun 5, 2021
@bogdandrutu
Copy link
Member

Please fix lint errors

@bogdandrutu
Copy link
Member

@odeke-em ping on the lint errors

@odeke-em odeke-em force-pushed the receiver-prometheus-pdataMetricFamily branch from b144672 to dc06ebc Compare June 8, 2021 10:38
@odeke-em odeke-em force-pushed the receiver-prometheus-pdataMetricFamily branch from dc06ebc to a32a210 Compare June 8, 2021 10:47
@odeke-em
Copy link
Member Author

odeke-em commented Jun 8, 2021

golangci-lint errors are unclear/phantom and pedantic -- max of 3 import groups which seems arbitrary and no way a Go convention, and it wasn't clear. However, I've addressed them. Thanks for the patience!

@tigrannajaryan
Copy link
Member

@odeke-em thanks a lot for working on this! Can you please update the PR description / commit message to reflect how much of the refactoring this does and what remains to be done.

tigrannajaryan
tigrannajaryan approved these changes Jun 8, 2021
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Will merge once the PR descr/commit message are updated.

@odeke-em
Copy link
Member Author

odeke-em commented Jun 8, 2021

@tigrannajaryan I've updated the PR description. The PR is composed of multiple commits so updating the commit message would require me squashing them directly into one then force pushing which would lose context. I believe perhaps that when you go to squash+merge, you'll be able to copy+paste the updated description. Thank you for the review!

@tigrannajaryan tigrannajaryan merged commit d69b479 into open-telemetry:main Jun 8, 2021
@tigrannajaryan
Copy link
Member

Thanks @odeke-em
I used the PR description as the commit message.

@odeke-em
Copy link
Member Author

odeke-em commented Jun 9, 2021

Awesome, thank you @tigrannajaryan!

@odeke-em odeke-em deleted the receiver-prometheus-pdataMetricFamily branch June 9, 2021 08:47
@gillg gillg mentioned this pull request Jun 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@dashpole dashpole dashpole approved these changes

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@alolita alolita Awaiting requested review from alolita

@bogdandrutu bogdandrutu Awaiting requested review from bogdandrutu

Assignees
No one assigned
Labels
waiting-for-author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@odeke-em @bogdandrutu @tigrannajaryan @Aneurysm9 @dashpole @alolita