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

Retrieve serverless build hash from nodes info API #1756

Merged
merged 7 commits into from
Aug 7, 2023

Conversation

gbanasiak
Copy link
Contributor

@gbanasiak gbanasiak commented Aug 3, 2023

This PR adds retrieval of build hash for serverless clusters from nodes info API (_nodes?filter_path=**.build_hash).

The PR introduces 2 configuration settings under driver section:

  • serverless.mode - equals true if Rally targets serverless cluster
  • serverless.operator - equals true if Elasticsearch user has operator privileges

Copy link
Member

@inqueue inqueue left a comment

Choose a reason for hiding this comment

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

I left one suggestion. Otherwise, another review round is not needed. LGTM

esrally/driver/driver.py Outdated Show resolved Hide resolved
@ebadyano
Copy link
Contributor

ebadyano commented Aug 4, 2023

Thank you for the change! Does it make sense to add a test in driver_test.py?

@gbanasiak
Copy link
Contributor Author

@ebadyano

Does it make sense to add a test in driver_test.py?

Sure, added in b9a2ab8. Please take a look.

@gbanasiak gbanasiak requested a review from dliappis August 4, 2023 11:00
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thank you. This seems to work but I have some questions around populating the retrieved field in the remote metric store.

if serverless_mode and serverless_operator:
build_hash = self.retrieve_build_hash_from_nodes_info(es_clients)
self.logger.info("Retrieved actual build hash [%s] from serverless cluster.", build_hash)
self.target.cluster_details["version"]["build_hash"] = build_hash
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the stats in the metric store?

There is some code in

rally/esrally/telemetry.py

Lines 1778 to 1781 in c3b04f4

revision = client_info["version"].get("build_hash", distribution_flavor)
# build version does not exist for serverless
distribution_version = client_info["version"].get("number", distribution_flavor)
self.metrics_store.add_meta_info(metrics.MetaInfoScope.cluster, None, "source_revision", revision)
and in my tests it results in

image

(correctly)

but

image

whereas I see in the logs Retrieved actual build hash [ad5c80e1b49f10b42d438253d7cc0b9753c156b9] from serverless cluster.

Additionally, shouldn't it also be collected by the node stats telemetry device?

Copy link
Contributor Author

@gbanasiak gbanasiak Aug 4, 2023

Choose a reason for hiding this comment

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

Telemetry device modifications are scoped separately in ES-6459, but after discussion with @dliappis we concluded the necessary changes to override build hash (revision) in ClusterEnvironmentInfo are small, so I went ahead and added da34a7d. PTAL.

Edit: Additional changes to telemetry devices will go into separate PRs.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Thanks for iterating. LGTM.

esrally/telemetry.py Show resolved Hide resolved
@gbanasiak gbanasiak merged commit a7387ae into elastic:master Aug 7, 2023
@gbanasiak gbanasiak deleted the serverless-build-hash branch August 7, 2023 08:15
@pquentin pquentin added this to the 2.9.0 milestone Aug 16, 2023
@pquentin pquentin added enhancement Improves the status quo :Config Config file format changes, new properties, ... labels Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Config Config file format changes, new properties, ... enhancement Improves the status quo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants