Skip to content

Fix rally-annotations index creation#1747

Merged
gbanasiak merged 2 commits intoelastic:masterfrom
gbanasiak:fix-annotations-template
Jul 13, 2023
Merged

Fix rally-annotations index creation#1747
gbanasiak merged 2 commits intoelastic:masterfrom
gbanasiak:fix-annotations-template

Conversation

@gbanasiak
Copy link
Copy Markdown
Contributor

@gbanasiak gbanasiak commented Jul 12, 2023

Addresses #1746.

The following naive fix reveals more problems:

diff --git a/esrally/resources/annotation-template.json b/esrally/resources/annotation-template.json
index 540bc589..fa62eaa2 100644
--- a/esrally/resources/annotation-template.json
+++ b/esrally/resources/annotation-template.json
@@ -1,6 +1,7 @@
 {
   "settings": {
-    "number_of_shards": 1
+    "index": {
+    }
   },
   "mappings": {
     "dynamic_templates": [

Output:

(.venv) grzegorz:elastic/rally% esrally add annotation --configuration-name=nightly-staging --track=elastic/security --message="test" --race-timestamp=20230712T000000Z

    ____        ____
   / __ \____ _/ / /_  __
  / /_/ / __ `/ / / / / /
 / _, _/ /_/ / / / /_/ /
/_/ |_|\__,_/_/_/\__, /
                /____/

[ERROR] Cannot add. module 'elasticsearch' has no attribute 'helpers'.

Stack trace:

2023-07-12 06:35:27,969 -not-actor-/PID:59902 esrally.rally ERROR A fatal error occurred while running subcommand [add].
Traceback (most recent call last):
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 107, in guarded
    return target(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/grzegorz/Documents/src/elastic/rally/.venv/lib/python3.11/site-packages/elasticsearch/_sync/client/utils.py", line 395, in wrapped
    raise ValueError(
ValueError: Couldn't merge 'body' with other parameters as it wasn't a mapping. Instead of using 'body' use individual API parameters

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/rally.py", line 1100, in dispatch_sub_command
    dispatch_add(cfg)
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/rally.py", line 865, in dispatch_add
    metrics.add_annotation(cfg)
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 1305, in add_annotation
    race_store(cfg).add_annotation()
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 1655, in add_annotation
    return self.es_store.add_annotation()
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 1777, in add_annotation
    self.client.create_index(index="rally-annotations", body=body)
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 71, in create_index
    return self.guarded(self._client.indices.create, index=index, body=body, ignore=400)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/grzegorz/Documents/src/elastic/rally/esrally/metrics.py", line 165, in guarded
    except elasticsearch.helpers.BulkIndexError as e:
           ^^^^^^^^^^^^^^^^^^^^^
AttributeError: module 'elasticsearch' has no attribute 'helpers'

Therefore the problem is not only the missing index object in annotations-template.json which is what #1746 suggests but also upgraded Python Elasticsearch client is more strict when evaluating body parameter in index create method.

This PR addresses the problem by aligning rally-annotations index creation handling with other indices, i.e. by using rally-annotations index template. This simplicity comes at a cost of slightly "wasteful" configuration if users decided to increase datastore.number_of_shards setting (rally-annotations is tiny).

I've tested the change in staging environment:

DELETE rally-annotations
esrally add annotation ...

Upgraded Python Elasticsearch client is more strict when evaluating
`body` parameter in index `create` method. This had broken
`rally-annotations` index creation in Elasticsearch metric store.

This commit fixes `rally-annotations` index creation adopting the
approach from other Rally indices which all use index templates.
@gbanasiak gbanasiak requested review from b-deam and ebadyano July 12, 2023 06:47
Copy link
Copy Markdown
Contributor

@ebadyano ebadyano left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Copy link
Copy Markdown
Member

@b-deam b-deam left a comment

Choose a reason for hiding this comment

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

LGTM. The extra shards for annotations isn't a big enough deal to bother refactoring the code to allow overrides per template IMHO.

@gbanasiak gbanasiak added the bug Something's wrong label Jul 13, 2023
@gbanasiak gbanasiak added this to the 2.8.1 milestone Jul 13, 2023
@gbanasiak gbanasiak merged commit 7d913f7 into elastic:master Jul 13, 2023
@gbanasiak gbanasiak deleted the fix-annotations-template branch July 13, 2023 08:08
@gbanasiak gbanasiak modified the milestones: 2.8.1, 2.9.0 Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something's wrong

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants