-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add documentation for TS source #134373
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
Add documentation for TS source #134373
Conversation
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
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 Kostas!
| [metric type](docs-content://manage-data/data-store/data-streams/time-series-data-stream-tsds.md#time-series-metric) | ||
| include: | ||
|
|
||
| - `LAST_OVER_TIME()`: applies to gauges and counters |
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.
counter fields suport rate only.
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.
Right, see #133295
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.
@kkrik-es this is looking really good! Thank you for drafting it.
I know it's still a draft, but I gave it a quick read and made small suggestions. @leemthompo we'll definitely want your eyes on this too 🙏
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Marci W <[email protected]>
|
@leemthompo I've updated the time-series functions as well and included them here. Please take a look, happy to split this in more PRs if it helps. |
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.
Made some suggestions for clarity. Thank you!
Other docs typically don't hyphenate time series, even when they should 🙃 but I didn't mark it everywhere because it's not super critical.
@leemthompo over to you for a more essential review
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
| These functions are implicitly evaluated per per time-series, with their results | ||
| then aggregated per grouping bucket using a secondary aggregation | ||
| function. More concretely, consider the following query: |
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.
@leemthompo going for conciseness, but possibly too far?
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
| **Best practices** | ||
|
|
||
| - Avoid mixing aggregation functions on different metrics in the same query, to | ||
| avoid interference between different time-series. For instance, if one metric |
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.
Claude's attempt to rewrite this 🤖 + some edits by me -- @kkrik-es WDYT? please check for accuracy!
Avoid aggregating multiple metrics in the same query when those metrics have different dimensional cardinalities. For example, in STATS max(rate(foo)) + rate(bar)), if foo and bar don't share the same dimension values, the rate for one metric will be null for some dimension combinations. Because the + operator returns null when either input is null, the entire result becomes null for those dimensions. Additionally, queries that aggregate a single metric can filter out null values more efficiently.
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.
Applied, thanks!
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.
unresolving this in reference to #134373 (comment)
(the code font didn't get applied as shown in this claude+me suggestion -- you can "edit" the comment to copy the markdown instead of plain text)
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
# Conflicts: # docs/reference/query-languages/esql/_snippets/functions/description/rate.md # docs/reference/query-languages/esql/kibana/definition/functions/rate.json # docs/reference/query-languages/esql/kibana/docs/functions/rate.md # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Idelta.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Increase.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Irate.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/Rate.java
Co-authored-by: Marci W <[email protected]>
docs/reference/query-languages/esql/functions-operators/time-series-aggregation-functions.md
Show resolved
Hide resolved
...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/aggregate/AvgOverTime.java
Outdated
Show resolved
Hide resolved
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.
Sorry I forgot to group my latest comments in a review :/
I think this is looking very good now and big thanks for all the iteration @kkrik-es. Once those final fixes are through, I'd consider this ready to merge from a writing perspective. It might be good to a final pair of technical eyeballs on this, because it's information rich!
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Liam Thompson <[email protected]>
Co-authored-by: Liam Thompson <[email protected]>
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
…ts.md Co-authored-by: Liam Thompson <[email protected]>
|
Thanks @leemthompo and @marciw for the review and the suggestions! @felixbarny mind taking another look, since this is much expanded? You can also check out the structure in the doc preview. |
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.
Just a couple of nits
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
| These functions are implicitly evaluated per time series, then aggregated by group using a secondary aggregation | ||
| function. For example: |
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: maybe improve the wording a bit on this.
| These functions are implicitly evaluated per time series, then aggregated by group using a secondary aggregation | |
| function. For example: | |
| These functions are implicitly evaluated per time series. | |
| A secondary secondary aggregation function, in combination with a `BY` grouping expression, allows to group by a set of dimensions. | |
| For example: |
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'll stick with Marci's version.
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/functions-operators/time-series-aggregation-functions.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/functions-operators/time-series-aggregation-functions.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/functions-operators/time-series-aggregation-functions.md
Outdated
Show resolved
Hide resolved
| such as `SUM()`, as outer aggregation functions. Using a time series aggregation | ||
| in combination with an inner function causes an error. For example, the |
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.
| such as `SUM()`, as outer aggregation functions. Using a time series aggregation | |
| in combination with an inner function causes an error. For example, the | |
| such as `SUM()`, as outer aggregation functions. | |
| Nesting time series aggregation functions causes an error. | |
| For example, the |
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.
Hm nesting is not very clear? Using them as inner functions can still be called "nesting".
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
docs/reference/query-languages/esql/_snippets/commands/layout/ts.md
Outdated
Show resolved
Hide resolved
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.
Thank you @kkrik-es, stellar work on those docs! I have only a minor comment for your consideration.
Additional minor notes on updates not included in this PR:
- Nitpick: On the time-series aggregation functions, why did we go with bucket on the examples instead of TBUCKET?
- Minor: On the time-series aggregation functions some of the descriptions are very laconic. We could maybe use some of the descriptions of the regular aggregation functions or link to them.
- A special case that I would propose to expand its description is the rate command as it is so critical for time series.
Approving on my side as everything mentioned on my comments is minor or optional.
| This query calculates the total rate of search requests (tracked by the `search_requests` counter) per host and hour. The `RATE()` | ||
| function is applied per time series in hourly buckets. These rates are summed for each | ||
| host and hourly bucket (since each host can map to multiple time series). |
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.
Optional suggestion for your consideration: I think that an example would benefit introducing this concept that may be new to some users (either here on on the examples section bellow)
Something that would showcase what happens on each step, using a diagram or tables with real dimension values. We do so for various important commands on the intro sections for ES|QL, so why not also here? I am thinking about this cause this is the most important concept, and then everything else is built on top of it.
Also, should we highlight a little bit more the special place of TBUCKET being used on both the inner and outer aggregations? We mention it but not state it explicitly
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 it's better to expand this in a blog post. I do think we should have more examples below to cover more use cases, without going into too many details of how things work.
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.
Sounds like a good plan, thank you Kostas!
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.
Thanks @kkrik-es!
Co-authored-by: Felix Barnsteiner <[email protected]>
|
@yannis-roussos I plan to follow up on expanding the documentation for rate, as it's special. The rest are more straightforward, but can be expanded as needed. Also, documentation for the time-series functions was added before we had the TBUCKET command.. Can be refined separately. |
|
Thanks all for the thorough reviews and recommendations! I'll keep iterating, will send some extra PRs. |
Covers how to apply time-series aggregations and the subtle differences with
STATSunder from.Documentation is currently nested under
TScommand, but it covers how aggregations inSTATSare affected. Another option is to move the time-series aggs in a dedicated section inSTATSand split the documentation betweenTSandSTATS.