search

elastic / rally

Macrobenchmarking framework for Elasticsearch
Apache License 2.0
37 stars 314 forks source link

Modify rally to allow multiple cars with complex configuration #1859

Closed favilo closed 4 months ago

favilo commented 5 months ago

We'd made some decisions in the past to only allow a single car with a python based configuration.

I'm not certain of the reasoning behind that, but I assume it was because this simplified the evaluation of cars.

This changes that to allow multiple cars to run with complex python based configuration. In particular, it allows us to run APM Tracing and X-Pack-Security simultaneously, which is something I need for benchmarking reasons.

gbanasiak commented 4 months ago

@favilo Do you think we should add esrally.utils.io and esrally.utils.process to mypy overrides too? My thinking is, if we start adding type hints somewhere, let's mypy do a proper check there, instead of a narrowed-down one. This will raise more mypy errors though, and the scope of PR will increase. Alternatively, we can retract type hint changes from those 2 modules, and add them in a separate PR.

favilo commented 4 months ago

@favilo Do you think we should add esrally.utils.io and esrally.utils.process to mypy overrides too? My thinking is, if we start adding type hints somewhere, let's mypy do a proper check there, instead of a narrowed-down one. This will raise more mypy errors though, and the scope of PR will increase. Alternatively, we can retract type hint changes from those 2 modules, and add them in a separate PR.

I will add those modules in, I like the idea of decreasing debt continuously. And I still need this branch around for my config to load properly

favilo commented 4 months ago

@gbanasiak I went a little crazy with those two files... Turned on a stricter mode of mypy than I absolutely needed. But this way, we can be sure we are future proof

gbanasiak commented 4 months ago

I'm testing simplified mypy config in https://github.com/elastic/rally/pull/1859/commits/2058474dc796f3e81f7bc4379041e82a57e1c414.

gbanasiak commented 4 months ago

https://github.com/elastic/rally/pull/1859/commits/56ef7e5224af543f5f8b2aaa06324c56926b3d23 changed file object type in FileOffsetTable from generic IO to byte-based IO, but files are opened in text-mode in this IO wrapper. The split() is called against str not bytes at runtime here: https://github.com/elastic/rally/blob/56ef7e5224af543f5f8b2aaa06324c56926b3d23/esrally/utils/io.py#L554

[..]
  File "/opt/buildkite-agent/builds/bk-agent-prod-gcp-1720474499404871070/elastic/rally-it/.nox/it-3-8/lib/python3.8/site-packages/esrally/utils/io.py", line 656, in skip_lines
    offset, remaining_lines = file_offset_table.find_closest_offset(number_of_lines_to_skip)

  File "/opt/buildkite-agent/builds/bk-agent-prod-gcp-1720474499404871070/elastic/rally-it/.nox/it-3-8/lib/python3.8/site-packages/esrally/utils/io.py", line 558, in find_closest_offset
    line_number, offset_in_bytes = (int(i) for i in line.strip().split(b";"))

TypeError: must be str or None, not bytes

Attempt to fix in https://github.com/elastic/rally/pull/1859/commits/f5125d262bb9bd3f6355d34110cae4cd3d7235d7