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

NaN values from Prometheus GoCollector breaking the Status API #4319

Closed
tsandall opened this issue Feb 1, 2022 · 3 comments
Closed

NaN values from Prometheus GoCollector breaking the Status API #4319

tsandall opened this issue Feb 1, 2022 · 3 comments
Labels
bug monitoring Issues related to decision log and status plugins

Comments

@tsandall
Copy link
Member

tsandall commented Feb 1, 2022

The Status API is broken in v0.37.0 because of an update to the Prometheus client library (v1.12.0 => v1.12.1). With v1.12.1 the GoCollector contains metrics that have NaN values that cannot be JSON serialized. As a result, the status messages that embed these metrics cannot be serialized.

I've filed an issue with the Prometheus client library to see what can be done. prometheus/client_golang#981. In the meantime we'll revert to the old version of the library.

Before close this issue we should:

  • Find out whether the metrics coming out of Prometheus are expected to always be JSON serializable (or if this is expected behaviour)
  • If some metrics cannot be JSON serialized, we need to exclude them from the Status API or make the Status API tolerant of them being set to NaN
  • Add an integration test that enables the Status API with the GoCollector metrics included. Our existing test suite didn't catch this because the GoCollector metrics are only included via the runtime package which doesn't contain integration tests for all of the plugins.
@tsandall tsandall added the bug label Feb 1, 2022
@tsandall tsandall added this to Backlog in Open Policy Agent via automation Feb 1, 2022
@tsandall tsandall added the monitoring Issues related to decision log and status plugins label Feb 9, 2022
@tsandall
Copy link
Member Author

@srenatus what's happening with this? Do we need to move to a different JSON serialization library? If so, it would be good to update this issue with the details.

@srenatus
Copy link
Contributor

#4324 should do the trick, I believe.

@srenatus
Copy link
Contributor

Fixed by #4324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug monitoring Issues related to decision log and status plugins
Projects
Development

No branches or pull requests

2 participants