-
Notifications
You must be signed in to change notification settings - Fork 88
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
node_disk_*: Keep a cumulative count of bytes read and written #37
base: main
Are you sure you want to change the base?
Conversation
The node exporter maintains this as a counter of the total bytes read and written since system boot. RDS enhanced metrics do not give us this information, instead presenting the number of bytes read and written in each monitoring interval. This means that the node exporter metrics we produce here were previously incorrect. Instead of directly exposing the information provided by the RDS metrics, maintain a counter to which we add the additional bytes read/written on each scrape. Also, ensure that we only process each event once, as duplicate processing now means that cumulative statistics are incorrect.
@@ -11,6 +11,27 @@ import ( | |||
"github.com/prometheus/client_golang/prometheus" | |||
) | |||
|
|||
var ( | |||
diskRead = prometheus.NewCounterVec( |
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.
diskRead
is a global variable (from gochecknoglobals
)
}, | ||
[]string{"region", "instance", "device"}, | ||
) | ||
diskWritten = prometheus.NewCounterVec( |
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.
diskWritten
is a global variable (from gochecknoglobals
)
I fixed the cuddling issues golangcibot complained about. However, the global variable usage is valid in this case as this is how |
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.
Hi, thanks for you contribution.
As far as I see the problem of this implementation that it collects total number of bytes for all instances and doesn't separate metrics by instances.
Tests are failing and merge conflicts there.
Please update PR or if you can't we will.
@stevenjm is this still an issue? maybe you could update PR or we can |
The node exporter maintains this as a counter of the total bytes read and written since system boot. RDS enhanced metrics do not give us this information, instead presenting the number of bytes read and written in each monitoring interval.
This means that the node exporter metrics we produce here were previously incorrect. Instead of directly exposing the information provided by the RDS metrics, maintain a counter to which we add the additional bytes read/written on each scrape.
Also, ensure that we only process each event once, as duplicate processing now means that cumulative statistics are incorrect.
Feedback wanted
This change currently does not honour per-instance labels. This is because a
CounterVec
does not provide a way to have different labels perCounter
.I'm not sure what the best way is to solve this. We could calculate the union set of labels on all instances and add those labels to the
CounterVec
. Alternatively, if there is a good way to synthesise a new metric with the value of an existing metric, we could use theCounterVec
to track the cumulative values and then create new metrics with added labels for export.A third option is that there might be a better way to implement this change altogether.
I'm interested in some feedback on what kind of solution would be accepted before I put more work into this.