-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Dfs query phase coordinator metric #136481
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
base: main
Are you sure you want to change the base?
Dfs query phase coordinator metric #136481
Conversation
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
Hi @chrisparrinello, I've created a changelog YAML for you. |
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.
Left a couple of comments, thanks!
private final Client client; | ||
private final AbstractSearchAsyncAction<?> context; | ||
private final SearchProgressListener progressListener; | ||
private long phaseStartTimeInNanos; |
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.
where do you set the initial value for this? I guess it's missing and tests don't catch it because we never verify that timing is sensible :)
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.
Whoops. Yeah it was missed. I'll add it.
} | ||
|
||
private void onFinish(AggregatedDfs dfs) { | ||
context.getSearchResponseMetrics().recordSearchPhaseDuration(getName(), System.nanoTime() - phaseStartTimeInNanos); |
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 this one, shouldn't we be able to store attributes like we do at the shard level? I may have forgotten to raise this in your previous PR. perhaps a follow-up for later to do so when possible?
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 think that would be best for a follow-up PR for later. I think the issue in some cases is having access to the SearchRequest and the list of localIndices. I think the SearchRequest object is easy to come by but the localIndices not as easy. If it is okay to pass in an empty array or null then we can take care of this sooner I think.
The query phase of a DFS search runs in a separate implementation. This change adds a new metric
es.search_response.took_durations.dfs_query.histogram
to measure that.