Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add status metrics on prometheus #4251
add status metrics on prometheus #4251
Changes from all commits
9f4187d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this? I don't think it's used, let's just keep this minimal. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented these methods here only to implement the interface prometheus.Registerer, If Prometheus already has an interface for registering, I prefer to use that instead of creating a specific for OPA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Thanks, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, do you think we'll need the ability to unregister a collector?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same explication above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not an expert with prometheus, so please bear with me on the naive question: Is it common to use gauges like that? From the docs,
Now, I'm not sure how "status" and the unix nano metrics below fit into this. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is common, as the Gauge is an int64 and can be set as any value it is perfect for timestamp.
For example:
https://github.com/kubernetes/kube-state-metrics/blob/master/docs/cronjob-metrics.md the metric kube_cronjob_next_schedule_time
https://github.com/prometheus/node_exporter the metric node_time_seconds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose ultimately, it depends on the types of metrics you'd like to put on a dashboard...
I think I can follow the argument wrt having the timestamps in guages, but what is
plugin_status_gauge
about? From what I can tell, we're setting the value to 1 and use the label to carry the status... is that useful? I guess I don't see what this adds over the other gauges...? should this be alast_status_change
timestamp? 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @srenatus , I haven’t made this clear what I was willing to do with each metric, my bad sorry.
The table below has how I use each metric (The queries are just examples).
"$instance", name="$bundle"})/1e+9)))'"$instance", name="$bundle"})/1e+9)))'"$instance", name="$bundle"})/1e+9)))'"$instance", name="$bundle"})/1e+9)))'Thinking about which questions The status metrics should answer, what questions would you do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For exemple part of my dash :