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/prometheusremotewrite] validate if scope is already present #36927

Merged
merged 12 commits into from
Jan 16, 2025
Merged

[receiver/prometheusremotewrite] validate if scope is already present #36927

merged 12 commits into from
Jan 16, 2025

Conversation

perebaj
Copy link
Contributor

@perebaj perebaj commented Dec 23, 2024

Description

Continuing the work done in this PR, addressing a TODO.

ArthurSens
ArthurSens reviewed Dec 24, 2024
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Good start! We're only missing a test and don't forget to add datapoints for the case where scopemetrics already exist!

.chloggen/prwreceiver-checkscope.yaml Outdated Show resolved Hide resolved
receiver/prometheusremotewritereceiver/receiver.go Outdated Show resolved Hide resolved
@perebaj
Copy link
Contributor Author

perebaj commented Dec 30, 2024

Good start! We're only missing a test and don't forget to add datapoints for the case where scopemetrics already exist!

Can you guide me in this test case? I'm really struggling to implement it.

@ArthurSens
Copy link
Member

Can you guide me in this test case? I'm really struggling to implement it.

Sure thing! What exactly is your doubt?

Currently, we have one test called TestTranslateV2, which receives a PRWv2 request and expects OtelMetrics. We should probably extend this one with a new test case that focuses on the correct translation of metrics with the scope labels. A new test with scope as a focus could also be an option!

ArthurSens
ArthurSens reviewed Jan 13, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Great job with the new test!

receiver/prometheusremotewritereceiver/receiver.go Show resolved Hide resolved
@perebaj perebaj marked this pull request as ready for review January 15, 2025 10:59
@perebaj perebaj requested a review from a team as a code owner January 15, 2025 10:59
@perebaj perebaj requested a review from ChrsMark January 15, 2025 10:59
ArthurSens
ArthurSens approved these changes Jan 15, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Just one nit, otherwise LGTM! Thank for the effort here, I'm sure it wasn't simple :)

receiver/prometheusremotewritereceiver/receiver.go Outdated Show resolved Hide resolved
ArthurSens
ArthurSens approved these changes Jan 16, 2025
View reviewed changes
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

Thanks!

@dashpole
Copy link
Contributor

prometheusremotewritereceiver/receiver.go:241:1: File is not properly formatted (gci)

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Jan 16, 2025
@songy23 songy23 merged commit 814307e into open-telemetry:main Jan 16, 2025
175 checks passed
@github-actions github-actions bot added this to the next release milestone Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArthurSens ArthurSens ArthurSens approved these changes

@dashpole dashpole dashpole approved these changes

@ChrsMark ChrsMark Awaiting requested review from ChrsMark ChrsMark is a code owner automatically assigned from open-telemetry/collector-contrib-approvers

Assignees

@songy23 songy23

Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheusremotewrite
Projects
None yet
Milestone
v0.118.0
Development

Successfully merging this pull request may close these issues.
4 participants
@perebaj @ArthurSens @dashpole @songy23