-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add time range bucketing attribute to APM shard search latency metrics #135524
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
Conversation
We recently added relevant attributes to the existing shard search latency metrics (elastic#134798). This commit introduces an additional attribute that analyzes the parsed time range filter against the @timestamp field and reports back whether it is within the last 15 minutes, last hour, last 12 hours, last day, last three days, last seven days, or last 14 days.
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
Hi @javanna, I've created a changelog YAML for you. |
if (hasDocValues()) { | ||
Query dvQuery = SortedNumericDocValuesField.newSlowRangeQuery(name(), l, u); | ||
query = new IndexOrDocValuesQuery(query, dvQuery); | ||
DateMathParser parser = resolveDateMathParser(forcedDateParser, lowerTerm, upperTerm); |
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 is just addressing some duplication on parsing the time ranges that I spotted while i was working on it, no functional change
if (includeLower == false) { | ||
++l; | ||
} | ||
if (fieldName.equals(DataStream.TIMESTAMP_FIELD_NAME)) { |
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 is the core of the change: as we parse the lower bound of a time range, if it is against the @timestamp
field, we save the parsed value in the context for later retrieval. This is good so we don't reparse dates multiple times which would be expensive.
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, thanks Luca!
SearchRequestAttributesExtractor.introspectTimeRange(randomLongBetween(threeDaysAgo, oneDayAgo), nowInMillis) | ||
); | ||
|
||
long sevenDaysAgo = nowInMillis - (7 * 24 * 60 * 60 * 1000); |
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.
nit: could use TimeValue.ofDays(7).toMilliseconds()
throughout this test for readability.
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.
Hey, i tried to stay away from that because otherwise it is a 1:1 copy of the code it's testing. Maybe not so important, but I'd prefer keeping the test super simple.
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.
We recently added relevant attributes to the existing shard search latency metrics (#134798). This commit introduces an additional attribute that analyzes the parsed time range filter against the @timestamp field and reports back whether it is within the last 15 minutes, last hour, last 12 hours, last day, last three days, last seven days, or last 14 days.