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

[prometheusreceiver] Handle staleNaN for up metric #9253

Merged
merged 5 commits into from
May 5, 2022

Conversation

gouthamve
Copy link
Member

@gouthamve gouthamve commented Apr 13, 2022

Fixes #5249

TODO:

  • Tests
  • Changelog

@gouthamve
Copy link
Member Author

Noooooope, closing this because NaN is never exposed via federate endpoint: https://github.com/prometheus/prometheus/blob/0b41fd6e716f3e8b79016f73b6dd77fdf5fcdced/web/federate.go#L123-L129

I have another idea as to what might cause #5249, will investigate that.

@gouthamve gouthamve closed this Apr 13, 2022
@gouthamve gouthamve deleted the fix-up-nan-scraping branch April 13, 2022 16:37
@gouthamve gouthamve restored the fix-up-nan-scraping branch April 13, 2022 18:59
@gouthamve gouthamve reopened this Apr 13, 2022
@gouthamve
Copy link
Member Author

DONOT MERGE: Depends on prometheus/prometheus#10588 being merged.

But once the above PR is merged, we will see the error in #5249 whenever a target goes away (I've verified it). I've fixed it here and will make the PR ready once upstream PR is merged.

@github-actions
Copy link
Contributor

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 28, 2022
When a target disappears, a StaleNaN is added. We should handle that case too.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve gouthamve marked this pull request as ready for review May 3, 2022 20:58
@gouthamve gouthamve requested a review from a team May 3, 2022 20:58
@gouthamve
Copy link
Member Author

@dashpole This is now ready for review. I am confident that #5249 will be fixed with this.

@github-actions github-actions bot removed the Stale label May 4, 2022
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.

Nice find

@dashpole dashpole added comp:prometheus Prometheus related issues bug Something isn't working labels May 4, 2022
@codeboten codeboten added the ready to merge Code review completed; ready to merge by maintainers label May 4, 2022
@codeboten codeboten merged commit 8772edf into open-telemetry:main May 5, 2022
@gouthamve gouthamve deleted the fix-up-nan-scraping branch May 5, 2022 18:43
djaglowski pushed a commit to djaglowski/opentelemetry-collector-contrib that referenced this pull request May 10, 2022
…9253)

* [prometheusreceiver] Handle staleNaN for up metric

When a target disappears, a StaleNaN is added. We should handle that case too.

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>

Co-authored-by: Alex Boten <aboten@lightstep.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working comp:prometheus Prometheus related issues ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid "up" metric
4 participants