-
Notifications
You must be signed in to change notification settings - Fork 51
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
Make metric names comply with Prometheus standards #44
Conversation
One up-front comment I have is that I believe the OST and MDT labels are necessary for the job stats as they give the specific target the job is on. For example, one node (Prometheus label name of |
@roclark That makes sense. Though, we could still add the MDT/OST information to more clearly distinguish the two sets of statistics. |
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.
Outside of the two comments I posted, this looks good to me!
sources/procfs.go
Outdated
{"job_cleanup_interval", "job_cleanup_interval_seconds", "Interval in seconds between cleanup of tuning statistics"}, | ||
{"job_stats", "job_read_samples_total", readSamplesHelp}, | ||
{"job_stats", "job_read_minimum_size_bytes", readMinimumHelp}, | ||
{"job_stats", "job_read_maximum_size_bytes", readMaximumHelp}, | ||
{"job_stats", "job_read_total", readTotalHelp}, |
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 want job_read_bytes_total
here? To me, it sounds like they want just _total
as a unit-less suffix. Since we have a unit of bytes, I believe we want to include that before the suffix. Could be I am misinterpreting the documentation though.
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.
From the documentation:
...should have a suffix describing the unit, in plural form. Note that an accumulating count has total as a suffix, in addition to the unit if applicable.
http_request_duration_seconds
node_memory_usage_bytes
http_requests_total (for a unit-less accumulating count)
process_cpu_seconds_total (for an accumulating count with unit)
So if anything, I'm worried that I'm underutilizing total...
sources/procfs.go
Outdated
{"job_stats", "job_write_maximum", writeMaximumHelp}, | ||
{"job_stats", "job_write_samples_total", writeSamplesHelp}, | ||
{"job_stats", "job_write_minimum_size_bytes", writeMinimumHelp}, | ||
{"job_stats", "job_write_maximum_size_bytes", writeMaximumHelp}, | ||
{"job_stats", "job_write_total", writeTotalHelp}, |
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.
Same as above, but with job_write_bytes_total
.
Definitely agree on adding the prefix. I think keeping the prefix and the label would probably be our best option. |
b108f04
to
ed864aa
Compare
Ok, for this round I actually just slapped OST/MDT/MDS/MGS in front of every metric to make the source explicit. Some slight changes by changing anything that references capacity to a '_capacity' metric instead of '_total'. |
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.
This pass looks a lot closer to what we will want to end up with! Great work!
sources/procfs.go
Outdated
{"job_stats", "ost_job_read_samples_total", readSamplesHelp}, | ||
{"job_stats", "ost_job_read_minimum_size_bytes", readMinimumHelp}, | ||
{"job_stats", "ost_job_read_maximum_size_bytes", readMaximumHelp}, | ||
{"job_stats", "ost_job_read_total", readTotalHelp}, |
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.
Don't forget the unit before the end: ost_job_read_bytes_total
sources/procfs.go
Outdated
{"job_stats", "ost_job_write_samples_total", writeSamplesHelp}, | ||
{"job_stats", "ost_job_write_minimum_size_bytes", writeMinimumHelp}, | ||
{"job_stats", "ost_job_write_maximum_size_bytes", writeMaximumHelp}, | ||
{"job_stats", "ost_job_write_total", writeTotalHelp}, |
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.
ost_job_write_bytes_total
sources/procfs.go
Outdated
{"lock_count", "ost_lock_count_total", "Number of locks"}, | ||
{"lock_timeouts", "ost_lock_timeout_total", "Number of lock timeouts"}, | ||
{"contended_locks", "ost_lock_contended_total", "Number of contended locks"}, | ||
{"contention_seconds", "ost_lock_contention_seconds", "Time in seconds during which locks were contended"}, |
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.
Is this a counter? As in, do we need to append _total
? Don't know enough about contention_seconds
off-hand to determine one way or another with certainty.
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.
Yeah, probably is...
ed864aa
to
3989c62
Compare
@roclark Updates are done. As we discussed, let this marinate for a while and we'll make a merge decision on Friday. |
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.
Decided to spend another 30 minutes looking closely at these names. Let me know if you have any thoughts.
sources/procfs.go
Outdated
{"tot_granted", "tot_granted", "Total number of exports that have been marked granted"}, | ||
{"tot_pending", "tot_pending", "Total number of exports that have been marked pending"}, | ||
{"blocksize", "ost_blocksize_bytes", "Filesystem block size in bytes"}, | ||
{"brw_size", "ost_brw_size_total", "Block read/write size in bytes"}, |
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.
Thinking back on this, brw_size
is a setting, so it should probably be ost_brw_size_bytes
since it isn't a counter.
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.
Agree.
sources/procfs.go
Outdated
{"brw_stats", "ost_pages_per_bulk_rw_total", pagesPerBlockRWHelp}, | ||
{"brw_stats", "ost_discontiguous_pages_total", discontiguousPagesHelp}, | ||
{"brw_stats", "ost_disk_io_now", diskIOsInFlightHelp}, | ||
{"brw_stats", "ost_io_time_ms", ioTimeHelp}, |
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.
IO Time
is a counter, so probably want to change to ost_io_time_ms_total
, yes?
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.
Agree.
sources/procfs.go
Outdated
{"brw_stats", "ost_io_time_ms", ioTimeHelp}, | ||
{"brw_stats", "ost_disk_io_total", diskIOSizeHelp}, | ||
{"degraded", "ost_degraded_now", "Binary indicator as to whether or not the pool is degraded - 0 for not degraded, 1 for degraded"}, | ||
{"filesfree", "ost_files_free_now_total", "The number of inodes (objects) available"}, |
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.
Might want to remove the _total
suffix since this isn't a counter value.
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.
Agree.
sources/procfs.go
Outdated
{"job_stats", "ost_job_get_info_total", getInfoHelp}, | ||
{"job_stats", "ost_job_set_info_total", setInfoHelp}, | ||
{"job_stats", "ost_job_quotactl_total", quotactlHelp}, | ||
{"kbytesavail", "ost_kilobytes_available", "Number of kilobytes readily available in the pool"}, |
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 want to add _now
to keep this consistent with others (such as line 136)?
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.
Agree.
sources/procfs.go
Outdated
{"job_stats", "ost_job_set_info_total", setInfoHelp}, | ||
{"job_stats", "ost_job_quotactl_total", quotactlHelp}, | ||
{"kbytesavail", "ost_kilobytes_available", "Number of kilobytes readily available in the pool"}, | ||
{"kbytesfree", "ost_kilobytes_free", "Number of kilobytes allocated to the pool"}, |
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.
Same as above, do we add _now
?
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.
Agree.
sources/procfs.go
Outdated
{"blocksize", "mgs_blocksize_bytes", "Filesystem block size in bytes"}, | ||
{"filesfree", "mgs_files_free_now_total", "The number of inodes (objects) available"}, | ||
{"filestotal", "mgs_file_capacity", "The maximum number of inodes (objects) the filesystem can hold"}, | ||
{"kbytesavail", "mgs_kilobytes_available", "Number of kilobytes readily available in the pool"}, |
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 append _now
for uniformity?
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.
Agree.
sources/procfs.go
Outdated
{"filesfree", "mgs_files_free_now_total", "The number of inodes (objects) available"}, | ||
{"filestotal", "mgs_file_capacity", "The maximum number of inodes (objects) the filesystem can hold"}, | ||
{"kbytesavail", "mgs_kilobytes_available", "Number of kilobytes readily available in the pool"}, | ||
{"kbytesfree", "mgs_kilobytes_free", "Number of kilobytes allocated to the pool"}, |
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 append _now
for uniformity?
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.
Agree.
sources/procfs.go
Outdated
{"kbytestotal", "kbytestotal", "Capacity of the pool in kilobytes"}, | ||
{"quota_iused_estimate", "quota_iused_estimate", "Returns '1' if a valid address is returned within the pool, referencing whether free space can be allocated"}, | ||
{"blocksize", "mds_blocksize_bytes", "Filesystem block size in bytes"}, | ||
{"filesfree", "mds_files_free_now_total", "The number of inodes (objects) available"}, |
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.
Again, mds_files_free_now_bytes
?
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.
Agree
sources/procfs.go
Outdated
{"blocksize", "mds_blocksize_bytes", "Filesystem block size in bytes"}, | ||
{"filesfree", "mds_files_free_now_total", "The number of inodes (objects) available"}, | ||
{"filestotal", "mds_file_capacity", "The maximum number of inodes (objects) the filesystem can hold"}, | ||
{"kbytesavail", "mds_kilobytes_available", "Number of kilobytes readily available in the pool"}, |
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.
Append _now
?
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.
Agree
sources/procfs.go
Outdated
{"filesfree", "mds_files_free_now_total", "The number of inodes (objects) available"}, | ||
{"filestotal", "mds_file_capacity", "The maximum number of inodes (objects) the filesystem can hold"}, | ||
{"kbytesavail", "mds_kilobytes_available", "Number of kilobytes readily available in the pool"}, | ||
{"kbytesfree", "mds_kilobytes_free", "Number of kilobytes allocated to the pool"}, |
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.
Append _now
?
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.
Agree
3989c62
to
5d14aed
Compare
@roclark All updated. |
Great work @joehandzik! Looks good to me at the moment, but I will do another pass-through this week before we merge just to make sure. |
Prometheus'
I just used it to check the metrics of the current
|
Also, Lustre's unit of
|
Maybe the mixed
The metric |
Reading the label rule Do not put the label names in the metric name, as this introduces redundancy and will cause confusion if the respective labels are aggregated away I wonder if the
(What about a lower-case |
Shouldn't these two metrics be unified into a single metric?
Like this i.e. with a
(See the |
Some metrics' values can decrease, too. These metrics should be of type Three examples:
|
@knweiss Thanks for taking a look at this! We'll take as much of this feedback as we can into account. |
@knweiss thanks for the review! Looking specifically at your last comment (converting counters to gauges where appropriate), I definitely agree with you, though I think that belongs in another PR as we will have to make a very refactor to the code to make this change. I will create an issue so we can keep track of this going forwards. Thanks again! |
@knweiss I think there are only a couple pieces of your feedback that I hesitate to address, and I figured I would clarify why that is:
Thanks for the pointer to the promtool functionality! I think several of those warning will be cleaned up by fixing issue #47 (many of those metrics will become gauges). The base unit issue is similar to my second point above. I'm trying to walk the line between the Prometheus naming spec and not totally confusing folks who have been using Lustre for years. |
5d14aed
to
d3500e1
Compare
29b04f2
to
4f2cb71
Compare
4f2cb71
to
17b4d70
Compare
Note that Travis is failing because the master currently doesn't have a |
I was partially correct - however, if you want to have this run against Travis, you will need to rebase against master again to include the |
938a7e8
to
fc0f098
Compare
Ok guys, now that the gauge code is pulled in I changed the metrics that I thought could technically go up or down over time (rather than constantly increasing). Take a look when you have a chance, @roclark and @knweiss. I'll wait on a merge until I hear from both of your or Tuesday of next week, whichever comes first. |
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.
Couldn't find anything else at the moment. I will take another look at it later on with fresh eyes to see if I missed anything, otherwise this looks good to me.
sources/procfs.go
Outdated
{"brw_stats", "disk_io_total", diskIOSizeHelp, s.counterMetric}, | ||
{"degraded", "degraded_now", "Binary indicator as to whether or not the pool is degraded - 0 for not degraded, 1 for degraded", s.gaugeMetric}, | ||
{"filesfree", "files_free_now", "The number of inodes (objects) available", s.gaugeMetric}, | ||
{"filestotal", "file_capacity", "The maximum number of inodes (objects) the filesystem can hold", s.counterMetric}, |
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.
This one looks like a candidate for gauge in my opinion.
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.
Looks like this one was double-confirmed by @knweiss running promtool check-metrics. I'll update it!
@joehandzik This is the updated output of
|
I would like to discuss the handling of
AFAICS using these in Grafana is cumbersome because you have to query each metric individually. The following would be much easier to handle (i.e. distinguishing the operations by label but with a single metric name):
Now you can use the single query Rebuilding this graph with the current metrics is cumbersome and error prone - or am I missing something? (I think one could argue that the current md_stats metrics are a case of namespace pollution, too) BTW: The metrics of the following eight md_stats operations are missing in the code right now:
|
@knweiss I'm not opposed to that change, it make sense to me. It's just a matter of tweaking our naming for some of the multi-metric file parsers (though @roclark might be upset because he just finished refactoring those all down to a single function!). Based on your thoughts, I think the job_stats files are also candidates for this sort of renaming (but this will be tricky for the OST code, since it will need to parse half of the file the old way and half of the file the new way). I'll discuss with @roclark and we'll come up with a plan. As always, thanks for taking a look! Regarding the missing operations, I'm not sure what happened. I assume you see those metrics in your environment? When we were developing this, we didn't actually have any instances of those metrics, but we may have done the parsing for md_stats prior to our testing with job schedulers (we're using Slurm for our testing). |
fc0f098
to
190a7e7
Compare
Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
190a7e7
to
37e20ae
Compare
@knweiss After talking with @roclark, we think the right way to handle issue #64 is to make those changes after this PR is complete. The Prometheus exporter documentation regarding labels definitely indicates erring on the side of minimizing labels (https://prometheus.io/docs/practices/instrumentation/#do-not-overuse-labels), so I think it's ok to have the master branch in a state where aggregating the MDT/job operations is somewhat painful. It'd put you and @mjtrangoni in a better place to start looking at dashboards, and we could close out this PR and migrate future discussion about what metrics should be refactored to use labels in a future PR. We'd likely have a new PR to address issue #64 out in the next couple days. Does that sound reasonable? If so, lemme know if you think we can merge this PR. |
@joehandzik That's fine with me. IMHO it's no problem if it takes some iterations and pull requests to get this right (and easy to use). (Let's continue the discussion regarding #64 over there. I've just added a comment.) |
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.
LGTM!
Signed-Off-By: Joe Handzik joseph.t.handzik@hpe.com
Note that we need to make several passes at this before calling it 100% good. This is what I came up with on Friday, it touches most metrics and attempts to conform to this: https://prometheus.io/docs/practices/naming/
Some metrics still may not be specific enough (especially for the job stats...maybe we move to job_mdt_* and job_ost_*, then drop the OST vs MDT label for those stats).
Anyway, thoughts would be appreciated. No big rush on this, we want to get this done by the end of the week probably but this shouldn't take more than a few hours from each of us this week.