Skip to content
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

Fix rally-annotations index creation #1747

Merged
merged 2 commits into from
Jul 13, 2023

Conversation

gbanasiak
Copy link
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.
Copy link
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
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