Use Elasticsearch's logging configuration

[danielmitterdorfer]

Historically Rally needed to modify Elasticsearch's logging configuration, e.g. to enable verbose logging for the IndexingMemoryController. We've removed the surrounding infrastructure in Rally a while ago. Therefore, Rally can just use the logging configuration that is shipped with Elasticsearch.

Implementation notes

Before applying the configuration in rally-teams, Rally wipes the Elaticsearch's configuration directory:

https://github.com/elastic/rally/blob/24dee9ed704e5c98fc2999a665853ba4b2941586/esrally/mechanic/provisioner.py#L185

Then, it applies the configuration; it writes files in append mode (this allows mixins to contribute to the configuration):

https://github.com/elastic/rally/blob/24dee9ed704e5c98fc2999a665853ba4b2941586/esrally/mechanic/provisioner.py#L156-L166

A possible solution is to delete all but the log4j2.properties file and then apply the config as is. We should not hardcode that name though but instead introduce a "allowlist" concept (files to keep from the original config). The allowlist would be defined in rally-teams and evaluated by Rally. For backwards-compatibility we should also explicitly skip copying any files on the allowlist.

We also have a dedicated provisioner for the Docker image which writes config files to a directory on the host and then provides each file via a mount:

https://github.com/elastic/rally/blob/24dee9ed704e5c98fc2999a665853ba4b2941586/esrally/mechanic/provisioner.py#L466-L476

Here, it is sufficient to only skip copying any file on the allowlist.

[pquentin]

Thanks! This make a lot of sense.

One question: would it make more sense to use a denylist instead? It would be more verbose today, but it would match with the list we have in rally-teams, which seems more explicit. And it won't break if Elasticsearch adds a new configuration file (even though that's unlikely).

[danielmitterdorfer]

I'd be fine either way as long as the file name is not hardcoded in Rally. I'd treat my summary above as a proposal / hint rather than a prescription how it must be done. :)

it won't break if Elasticsearch adds a new configuration file (even though that's unlikely).

In fact, I would prefer if benchmarks break on a new configuration file because it explicitly alerts us of a change that might influence benchmarks so we need to make a concious decision how to handle it.

[pquentin]

A new configuration file would be in the release notes, hopefully? However, https://github.com/elastic/elasticsearch/pull/61474 was only mentioned in passing in https://www.elastic.co/guide/en/elasticsearch/reference/7.10/release-notes-7.10.0.html. Nothing in the release notes warns users about updating their log4j configuration.

I'm wondering because the natural evolution of my suggestion is to just overwrite the files that are in rally-teams instead of having to maintain a list at all. Would that make sense?

[danielmitterdorfer]

A new configuration file would be in the release notes, hopefully?

Even if that would be the case, we benchmark against master so by the time we see the release notes it's likely already too late?

[...] natural evolution of my suggestion is to just overwrite the files that are in rally-teams instead

I like the direction. We'd rather not break our users without any chance for them to upgrade so for a grace period I still think that we need to keep log4j2.properties in rally-teams and treat it specially. I am also not certain whether I fully understand how your proposal would work with the Docker image?

[pquentin]

I am also not certain whether I fully understand how your proposal would work with the Docker image?

I don't know what makes Docker different here, sorry.

[danielmitterdorfer]

I don't know what makes Docker different here, sorry.

The config files are already embedded in the Docker image. I think it's sufficient that we just don't provide a file outside the container and mount it. So it's just the semantics of "overwriting" files in rally-teams are different but now that I think of it more I don't think that's a problem.

Nevertheless, we still have the issue with backwards-compatibility, both for Docker and the tar.gz distribution. So I still think a deny / allowlist concept is necessary, at least for a transition period.