Add time range bucketing attribute to APM took time latency metrics #135549
Conversation
This is similar to elastic#135524, but adding the attribute to the took time latency metric. That requires a bit of ceremony as the took time metric is recorded on the coordinating node, while the time range filter is parsed on each shard. We don't have mappings available on the coord node, which are needed to parse dates on the coord node. Thus we need to rely on date parsing done on the data nodes, which requires sending back the parsed value to the coord node, performing some simple reduction on it, and adding it back to the search response.
javanna
added
>enhancement
:Search Foundations/Search
elasticsearchmachine
added
the
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @javanna, I've created a changelog YAML for you. |
| ) | ||
| ) { | ||
| QueryPhase.addCollectorsAndSearch(rankSearchContext); | ||
| QueryPhase.addCollectorsAndSearch(rankSearchContext, null); |
There was a problem hiding this comment.
I will look into this as a follow-up, need to write a test that leverages this and see if it makes sense to track timestamp from for it too, it probably does.
There was a problem hiding this comment.
Looking deeper at this, I think we are fine not tracking the time range from in this case. It's only used when multiple ranking steps are required. I don't think that we can reliably track the time range from in that case, as each sub-query may have its own independent filters. It's not that helpful either because using time range filters combined with ranking is a bit of an edge case that we don't want to focus on at the moment.
| assertEquals("hits_only", attributes.get("query_type")); | ||
| assertEquals("_score", attributes.get("sort")); | ||
| assertEquals("pit", attributes.get("pit_scroll")); | ||
| } |
There was a problem hiding this comment.
As a follow-up, I have more work to do around retrievers, as they optionally hold a query too. Need to walk the retrievers tree like I do for queries, and add more tests.
| } else { | ||
| this.rangeTimestampFrom = Math.min(rangeTimestampFrom, this.rangeTimestampFrom); | ||
| } | ||
| } |
There was a problem hiding this comment.
I realized that we need this in QueryRewriteContext, that this extends, to do the same tracking when we parse the time range filter as part of query rewrite. There's cases when the range gets rewritten to a range with no bounds (match all), for which we still want to bucket the request based on its original time range.
server/src/main/java/org/elasticsearch/action/search/SearchResponse.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/query/BoolQueryBuilder.java
Outdated
Show resolved
Hide resolved
| // range queries may get rewritten to match_all or a range with open bounds. Rewriting in that case is the only place | ||
| // where we parse the date and set it to the context. We need to propagate it back from the clone into the original context |
There was a problem hiding this comment.
❤️ this is very useful and a true comment, documenting the why. Thank you!
server/src/main/java/org/elasticsearch/search/query/QueryPhase.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java
Outdated
Show resolved
Hide resolved
….java Co-authored-by: Andrei Dan <[email protected]>
…hResult.java Co-authored-by: Andrei Dan <[email protected]>
javanna
added
the
auto-backport
javanna
removed
the
auto-backport
💔 Backport failedThe backport operation could not be completed due to the following error: You can use sqren/backport to manually backport by running |
There's situations where the time range filter is provided as part of the retriever tree. In that case, we capture the time range filter from while parsing it, but we don't do the corresponding introspection of the retriever tree to extract which fields were the time range filters made against. This commit introduces a very basic introspection of retrievers and tests around it, expanding on #135549.
This is similar to #135524, but adding the time range bucketing attribute to the took time latency metric.
That requires a bit of ceremony as the took time metric is recorded on the coordinating node, while the time range filter is parsed on each shard. We don't have mappings available on the coord node, which are needed to parse dates. Thus we need to rely on date parsing done on the data nodes, which requires sending back the parsed value to the coord node, performing some simple reduction on it, and adding it back to the search response.
This also includes two improvements to the existing mechanism: