Skip to content

Include 'time_series' aggregation tests#351

Merged
salvatore-campagna merged 4 commits intoelastic:masterfrom
salvatore-campagna:feature/time-series-aggs
Dec 13, 2022
Merged

Include 'time_series' aggregation tests#351
salvatore-campagna merged 4 commits intoelastic:masterfrom
salvatore-campagna:feature/time-series-aggs

Conversation

@salvatore-campagna
Copy link
Copy Markdown
Contributor

@salvatore-campagna salvatore-campagna commented Dec 6, 2022

Include Rally tests for time_series aggregations.

Resolves #350

Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I do think we need to add: https://github.com/elastic/rally-tracks/pull/348/files#diff-fd03625f346296b8349bb933b96aca0e9bd6d6144a954667c44f25e246b67e79R7

Otherwise we run into maximum number of allowed bucket limit while running the tsdb track.

"size": 1000
"aggs": {
"ts": {
"time_series": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also add a pipeline aggregator here. Since the next change, we're trying to do is pushing down the pipeline aggregator computation to time series aggregator on data node. With that change the response of the time_series agg would change too, it will not return time series buckets but the result of the associated pipeline aggregations.

I think a sum pipeline aggregator is ok here.

Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the max bucket limit needs to be increased here, LGTM otherwise.

Copy link
Copy Markdown
Contributor

@DJRickyB DJRickyB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestion, otherwise this LGTM

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>
@salvatore-campagna salvatore-campagna merged commit 1209733 into elastic:master Dec 13, 2022
b-deam added a commit to b-deam/rally-tracks that referenced this pull request Dec 13, 2022
b-deam added a commit that referenced this pull request Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend the TSDB challenge to include time_series aggregations

3 participants