Add label to io_threaded_fallbacks to categorize operations #108
Conversation
src/core/thread_pool.hh
Outdated
| @@ -27,9 +27,53 @@ namespace seastar { | |||
|
|
|||
| class reactor; | |||
|
|
|||
| enum class submit_reason { | |||
There was a problem hiding this comment.
(should probably document each reason as well)
| uint64_t process_operation{0}; | ||
| uint64_t unknown{0}; | ||
|
|
||
| void record_reason(submit_reason reason) { |
There was a problem hiding this comment.
I think it is better to make this "generic" (i.e., no need for O(n) clauses for n reasons), e.g., using an array of counters and indexing with the enum value.
There was a problem hiding this comment.
Oooh, I like that idea. Will switch the code over to that.
| } | ||
| } | ||
|
|
||
| uint64_t count_for(submit_reason reason) const { |
| return inter_thread_wq.submit<T>(std::move(func)); | ||
| } | ||
| uint64_t operation_count() const { return _aio_threaded_fallbacks; } |
There was a problem hiding this comment.
Why is this deleted? It could still be implemented with the same semantics as before, right?
There was a problem hiding this comment.
Yeah, summing all the reasons will have the same semantics as before. It's just not used anywhere else in the codebase so I removed it as dead code.
There was a problem hiding this comment.
I see, because the old metric is replaced entirely with the one the labels, makes sense. Not sure if seastar team will buy into not keeping the old metric (w/o labels) around but let's see.
src/core/thread_pool.hh
Outdated
| @@ -27,9 +27,53 @@ namespace seastar { | |||
|
|
|||
| class reactor; | |||
|
|
|||
| enum class submit_reason { | |||
There was a problem hiding this comment.
(should probably document each reason as well)
ballard26
force-pushed
the
metrics-for-thread-pool
branch
from
57dc8bb
to
4974a41
Compare
ballard26
force-pushed
the
metrics-for-thread-pool
branch
from
4974a41
to
646adfb
Compare
| sm::description("Total number of io-threaded-fallbacks operations")), | ||
|
|
||
| io_fallback_counter("aio_fallback", submit_reason::aio_fallback), | ||
| // total_operations value:DERIVE:0:U |
There was a problem hiding this comment.
do you even understand these comments? I sure don't.
There was a problem hiding this comment.
Haha, it's collectd stuff: https://www.collectd.org/documentation/manpages/types.db.html#:~:text=db%20file%20contains%20collectd's%20metric,bytes%E2%80%9D%20and%20%E2%80%9Ctotal_bytes%E2%80%9D.
I was wondering the same in the past
| @@ -27,9 +27,36 @@ namespace seastar { | |||
|
|
|||
| class reactor; | |||
|
|
|||
| // Reasons for why a function had to be submitted to the thread_pool | |||
| enum class submit_reason : size_t { | |||
| // Used for aio operations what would block in `io_submit`. | |||
There was a problem hiding this comment.
Just to clarify, isn't it actually "used as a fallback for io operations that failed to execute in a non-blocking manner in their first submission to io_submit"?
Maybe it's splitting hairs but the way it's written makes it sounds like we know that some operation will block and then we can redirect them to the thread pool, but it's actually based on what happened on a per-op basis during submission, right?
There was a problem hiding this comment.
Fair, I probably removed too much context in an attempt to make the comment succinct. Will add some back.
The main purpose of this PR is to allow us to know how many of the operations submitted to the thread pool are from failed aio reads/writes.