Record search metrics on cancelation #5743
Conversation
|
Writing a custom future would be much cleaner: |
| let label_values = match leaf_search_response_reresult { | ||
| Ok(Ok(_)) => ["success"], | ||
| _ => ["error"], | ||
| }; | ||
| SEARCH_METRICS | ||
| .leaf_search_targeted_splits | ||
| .with_label_values(label_values) | ||
| .observe(num_splits as f64); | ||
|
|
There was a problem hiding this comment.
Was there any specific reason why this was measured per index here?
rdettai
force-pushed
the
cancelation-search-metrics
branch
2 times, most recently
from
082adcb to
81b6b7f
Compare
Good point, I updated the PR to use a future. For the root search the state machine is a bit more complicated because the request can also fail/cancel during planning (fetch of the split metadata). I did some refactoring to have a clearer split between the I also refactored a few functions names to make it clearer the granularity at which we are working (multi-index vs single doc mapping) |
| (RootSearchMetricsStep::Plan, Some(false)) => (0, "plan-error"), | ||
| (RootSearchMetricsStep::Plan, None) => (0, "plan-cancelled"), |
There was a problem hiding this comment.
These extra statuses seem valuable, but we should also try to avoid creating too many series. No strong opinion on whether we should have these or not.
(On that topic, I think root_search_requests_total is actually redundant because root_search_request_duration_seconds is a histogram and that creates a _count series)
rdettai
force-pushed
the
cancelation-search-metrics
branch
from
81b6b7f to
86b6611
Compare
Description
This PRs moves
[root|leaf]_search_requests_total[root|leaf]_search_request_duration_seconds[root|leaf]_search_targeted_splitsinto tasks so that they are recorded even on future cancelation. This will help having a more accurate view on queries that timeout.How was this PR tested?
Describe how you tested this PR.