Include a new downsampling operation by salvatore-campagna · Pull Request #1574 · elastic/rally

salvatore-campagna · 2022-09-07T13:33:03Z

The new downsampling operation allows Rally to hit the Elasticsearch /<source-index>/_downsample/<target-index> endpoint to start a downsampling operation on the source index. Expected parameters include:

fixed_interval: the aggregation granularity using the same interval definition used by date histograms
source-index: the index to downsample applying the aggregation
target-index: the index created as a result of the aggregation on the timestamp field

Example: aggregate the index test-source-index on the timestamp field using a 1 minute interval and store the result in a new index called test-target-index.

{
  "name": "downsample",
  "operation": {
    "operation-type": "downsample",
    "fixed-interval": "1m",
    "source-index": "test-source-index",
    "target-index": "test-target-index"
  }
}

@timestamp

The new downsampling operation allows Rally to hit the Elasticsearch /<source-index>/_rollup/<target-index> endpoint to start a downsampling operation on the source index. Expected parameters include: * fixed_interval: the aggregation granularity using the same interval definition used by date histograms * source-index: the index to downsample applying the aggregation * target-index: the index created as a result of the aggregation on the @timestamp field

esrally/driver/runner.py

docs/track.rst

esrally/driver/runner.py

We would like to benchmark the downsample operation so we make it a non-administrative operation. Also we don't want it to be retried if it fails to avoid latency to be affected by retries.

DJRickyB

Left some suggestions.

One optional thing to consider that might be cool/useful is to add a parameter source for this operation, that can provide defaults for source_index and target_index for ease-of-use. We have helper function params.get_target(track, params) for this purpose in param sources, and your target-index could just default to some fixed pattern like f"{source-index}_{fixed-interval}"

docs/track.rst

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>

salvatore-campagna · 2022-09-09T07:18:37Z

@elasticmachine run rally/rally-tracks-compat

esrally/driver/runner.py

salvatore-campagna · 2022-09-14T09:26:16Z

The CI is failing with
XFAIL (index deletion bug under investigation)

pquentin · 2022-09-14T09:31:00Z

This means that test is now passing! It should be removed from elastic/rally-tracks@117f306 after merging this pull request. Or we could change that line in rally-tracks to @pytest.mark.xfail(reason="index deletion bug under investigation", strict=False).

See https://docs.pytest.org/en/7.1.x/how-to/skipping.html for details on that pytest fixture.

esrally/track/params.py

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>

esrally/track/params.py

DJRickyB · 2022-09-15T12:03:01Z

docs/track.rst

+""""""""""
+
+* ``fixed_interval`` (optional, defaults to ``1h``): The aggregation interval key defined as in `https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-bucket-datehistogram-aggregation.html#fixed_intervals`.
+* ``source-index`` (optional, defaults to ``tsdb-index``): The index containing data to aggregate which includes a ``@timestamp`` field. Note that this index should be marked read-only prior to the execution of this operation.


Suggested change

* ``source-index`` (optional, defaults to ``tsdb-index``): The index containing data to aggregate which includes a ``@timestamp`` field. Note that this index should be marked read-only prior to the execution of this operation.

* ``source-index`` (optional): The index containing data to aggregate which includes a ``@timestamp`` field. Note that this index should be marked read-only prior to the execution of this operation. If there is only one index defined in the ``indices`` of the track definition, that will be used as the default.

DJRickyB · 2022-09-15T12:33:40Z

tests/track/params_test.py

+        assert p["source-index"] == "tsdb"
+        assert p["target-index"] == "tsdb-1m"
+
+    def test_downsample_empty_params(self):


Not sure what this test buys us? Checked with a debugger and captured the value of p:

{'fixed-interval': '1h', 'source-index': None, 'target-index': 'None-1h', 'request-timeout': None, 'headers': None, 'opaque-id': None}

Might be better instead to have the Track constructor call include an indices param with value like [{"name": "test-source-index", "body": "index.json"}] and assert that source-index gets provided as test-source-index

DJRickyB · 2022-09-15T12:34:14Z

tests/track/params_test.py

+
+        p = source.params()
+
+        assert p["fixed-interval"] != ""


This should be the documented default, and should need changing if the default changes

DJRickyB

Still have 3 "unresolved conversations" on this PR, relating to documentation update and hardening testing the defaults

DJRickyB · 2022-09-15T13:16:46Z

tests/track/params_test.py

+        p = source.params()
+
+        assert p["fixed-interval"] == "1h"
+        assert p["source-index"] != ""


To put my above comment slightly differently, this assert only passes because the source-index is None instead of "", technically different but not a going concern. You've covered my larger concern in test_downsample_default_index_param, where you ensure there's a default when there is supposed to be a default, so perhaps this can just be deleted and the test is left to assert the default fixed-interval?

DJRickyB

This LGTM, thank you for iterating

salvatore-campagna requested review from michaelbaamonde and pquentin September 7, 2022 13:34

salvatore-campagna mentioned this pull request Sep 7, 2022

Include metric types in tsdb index and template mappings elastic/rally-tracks#316

Merged

salvatore-campagna added 2 commits September 7, 2022 15:36

fix: documentation format violations

126e99c

fix: documentation format violations

54b9829

salvatore-campagna commented Sep 7, 2022

View reviewed changes

esrally/driver/runner.py Outdated Show resolved Hide resolved

salvatore-campagna requested review from DJRickyB and dliappis September 7, 2022 18:03

DJRickyB assigned salvatore-campagna Sep 7, 2022

DJRickyB added enhancement Improves the status quo highlight A substantial improvement that is worth mentioning separately in release notes labels Sep 7, 2022

DJRickyB added this to the 2.6.1 milestone Sep 7, 2022

DJRickyB reviewed Sep 7, 2022

View reviewed changes

docs/track.rst Outdated Show resolved Hide resolved

docs/track.rst Outdated Show resolved Hide resolved

esrally/driver/runner.py Outdated Show resolved Hide resolved

esrally/driver/runner.py Outdated Show resolved Hide resolved

salvatore-campagna changed the title ~~feature: include a new downsampling operation~~ Include a new downsampling operation Sep 8, 2022

salvatore-campagna added 9 commits September 8, 2022 10:56

fix: use f-string for path

61ade59

fix: use 'downsample'

edf2e08

fix: use 'downsample' instead of 'downsampling'

81db5c6

fix: get rid of the body parameter

1446d12

fix: make Downsample operation administrative

b81526d

fix: remove body parameter

433726c

fix: make Downsample non-administrative

97e2922

We would like to benchmark the downsample operation so we make it a non-administrative operation. Also we don't want it to be retried if it fails to avoid latency to be affected by retries.

fix: remove body parameter

e4a8d81

test: fix endpoint and do not require body

d813ce0

salvatore-campagna requested review from DJRickyB and removed request for michaelbaamonde and pquentin September 8, 2022 13:11

DJRickyB suggested changes Sep 8, 2022

View reviewed changes

docs/track.rst Outdated Show resolved Hide resolved

docs/track.rst Outdated Show resolved Hide resolved

docs/track.rst Outdated Show resolved Hide resolved

docs/track.rst Outdated Show resolved Hide resolved

salvatore-campagna and others added 2 commits September 8, 2022 17:04

Update docs/track.rst

01d0d75

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>

Update docs/track.rst

571b712

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>

b-deam mentioned this pull request Sep 13, 2022

Add build subcommand #1576

Merged

fix: do not fail if index does not exist

693a9d3

salvatore-campagna commented Sep 14, 2022

View reviewed changes

esrally/driver/runner.py Outdated Show resolved Hide resolved

Merge branch 'master' into feature/downsampling-operation

476a573

DJRickyB reviewed Sep 15, 2022

View reviewed changes

esrally/track/params.py Outdated Show resolved Hide resolved

salvatore-campagna requested review from DJRickyB and removed request for dliappis September 15, 2022 10:19

salvatore-campagna and others added 2 commits September 15, 2022 12:47

fix: use get_target instead of static default value

d5e5ffe

Co-authored-by: Rick Boyd <boyd.richardj@gmail.com>

fix: remove ignore_unavailable param

9d1c257

salvatore-campagna commented Sep 15, 2022

View reviewed changes

esrally/track/params.py Outdated Show resolved Hide resolved

salvatore-campagna added 2 commits September 15, 2022 12:56

fix: params.get_target attribute does not exist

cad9f53

test: use index instead of source-index

7e063e2

DJRickyB reviewed Sep 15, 2022

View reviewed changes

fix: support both index and source-index params

ad1a9a6

DJRickyB reviewed Sep 15, 2022

View reviewed changes

salvatore-campagna requested a review from DJRickyB September 15, 2022 12:53

fix: explicitly check the default fixed_interval

104a715

DJRickyB suggested changes Sep 15, 2022

View reviewed changes

salvatore-campagna added 2 commits September 15, 2022 15:08

test: check both default index and source index

21cc596

docs: update source-index param documentation

029babb

salvatore-campagna requested a review from DJRickyB September 15, 2022 13:11

DJRickyB reviewed Sep 15, 2022

View reviewed changes

salvatore-campagna requested a review from DJRickyB September 15, 2022 13:20

fix: do not assert source-index

86e05ee

DJRickyB approved these changes Sep 15, 2022

View reviewed changes

salvatore-campagna merged commit af0c464 into elastic:master Sep 19, 2022

	* ``source-index`` (optional, defaults to ``tsdb-index``): The index containing data to aggregate which includes a ``@timestamp`` field. Note that this index should be marked read-only prior to the execution of this operation.
	* ``source-index`` (optional): The index containing data to aggregate which includes a ``@timestamp`` field. Note that this index should be marked read-only prior to the execution of this operation. If there is only one index defined in the ``indices`` of the track definition, that will be used as the default.

Conversation

salvatore-campagna commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DJRickyB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

salvatore-campagna commented Sep 9, 2022

Uh oh!

Uh oh!

salvatore-campagna commented Sep 14, 2022

Uh oh!

pquentin commented Sep 14, 2022

Uh oh!

Uh oh!

Uh oh!

DJRickyB Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJRickyB Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

DJRickyB Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

DJRickyB left a comment

Choose a reason for hiding this comment

Uh oh!

DJRickyB Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

DJRickyB left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

salvatore-campagna commented Sep 7, 2022 •

edited

Loading

DJRickyB Sep 15, 2022 •

edited

Loading