Skip to content

[prometheuswritereceiver] target info #38812

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

Merged
merged 32 commits into from
Apr 24, 2025
Merged

Conversation

perebaj
Copy link
Contributor

@perebaj perebaj commented Mar 19, 2025

Description

Cache target_info metrics so it can be used to populate metrics' Resource Attributes.

Link to tracking issue

Partially fixes #37277

@perebaj perebaj changed the title test cases [prometheuswritereceiver] target info Mar 19, 2025
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, but there are a few things not going in the right direction. Added a few comments below

Copy link
Contributor

github-actions bot commented Apr 8, 2025

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

@github-actions github-actions bot added the Stale label Apr 8, 2025
@perebaj
Copy link
Contributor Author

perebaj commented Apr 21, 2025

@ArthurSens I adapted the tests to use the forked version that fixes that snappy compression. But now I think that we have a different problem.

How we will validate the multiple requests' behavior? This because, the way that you explain to me that this code should be done, is using the MockConsumer. The issue with this approach, is if we use a unit test, we won't be able to have the snappy working, because the middleware it's just initialized when we start the real Server, like you did here.

Besides that, I think that in the response of the /api/v1/write we don't have access to the translated metric. So, how can use validate that the target_info is working between different requests?

@perebaj
Copy link
Contributor Author

perebaj commented Apr 21, 2025

@ArthurSens I adapted the tests to use the forked version that fixes that snappy compression. But now I think that we have a different problem.

How we will validate the multiple requests' behavior? This because, the way that you explain to me that this code should be done, is using the MockConsumer. The issue with this approach, is if we use a unit test, we won't be able to have the snappy working, because the middleware it's just initialized when we start the real Server, like you did here.

Besides that, I think that in the response of the /api/v1/write we don't have access to the translated metric. So, how can use validate that the target_info is working between different requests?

The answer was in my front of me the whole time

5a08906#diff-946c2dc5f6b3b6d865be9b13c57625f05b095c373d8dcc960fa4357985903058R519

I can pass the mockConsumer for the factory.CreateMetric 😊. Test done and working!

@github-actions github-actions bot removed the Stale label Apr 22, 2025
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.

Awesome progress, I think we're almost done :)

A few comments

@perebaj perebaj marked this pull request as ready for review April 23, 2025 11:25
@perebaj perebaj requested a review from a team as a code owner April 23, 2025 11:25
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 very small comment, otherwise LGTM!

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.

Thank you! The hardest part of the mentorship is done 🎉

@ArthurSens ArthurSens added the ready to merge Code review completed; ready to merge by maintainers label Apr 23, 2025
@mx-psi mx-psi requested a review from atoulme April 23, 2025 14:00
@atoulme atoulme merged commit be97cf9 into open-telemetry:main Apr 24, 2025
181 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 24, 2025
songy23 pushed a commit that referenced this pull request Jun 2, 2025
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description

Adding the obs report component

The discussion to add this component started
[here](#38812 (comment))

The last commit the we reviewed before I screw up the whole branch

https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/39828/files/13688834d2674c61ced371b5500abb4e901e8183

---------

Co-authored-by: Arthur Silva Sens <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/prometheusremotewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prometheus Remote-Write receiver Alpha
4 participants