Repository: elastic/rally

Issue:
Should we force Rally to clean up SSL connections?
During some recent benchmarking work we discovered [that a bug in how Elasticsearch handles the closing of existing SSL connections when TLS 1.3 is used](https://github.com/elastic/elasticsearch/issues/76642) can end up manifesting itself as a memory leak in Rally, and given enough time will eventually cause the benchmark to fail as the host machine memory utilisation grows to 100% and the OOMKiller is invoked. 

An example of this can be seen in the below screenshot:
![image](https://user-images.githubusercontent.com/54515790/159850699-913f2de1-463e-4413-a94c-74d5c7b022bb.png)


The bug isn't yet fixed in Elasticsearch, and may not be for a while. Luckily for us the workaround for this is very simple. [All that is needed is to set the client opt `enable_cleanup_closed: true`](https://esrally.readthedocs.io/en/stable/command_line_reference.html#client-options). 

So, given how simple this fix is, should we consider changing our current default of `false` to `true`? 
https://github.com/elastic/rally/blob/45e43e2d7165712e9e497a9beb3f65e429a7d23c/esrally/async_connection.py#L209 

Acknowledging both the fact that environments that do leak connections will almost definitely see an increase in performance, and that we haven't got any baseline for environments that do not currently suffer from this issue. Whether an environment is affected or not depends both on the version of Elasticsearch and whether or not TLS 1.3 is in use.

Discuss.




Discussion:
pquentin: Actually I think this only fixes the async case, not the sync one.
pquentin: @elasticmachine run tests please
pquentin: @elasticmachine run tests please
pquentin: > Actually I think this only fixes the async case, not the sync one. I'll fix it and add an unit test.

Actually async only is fine because `enable_cleanup_closed` only applies to async anyway. I added a test. I also ensured that:

 * a large benchmark did not leak connection and performed the same way
 * we introduced this setting in the past to set it to True in the presence of many clients (and not to set it to False)

I have a test currently running to make sure our nightly benchmarks also perform the same way.

I also had to fight a bit with a timings issue that failed in CI because we sleeped a bit more than expected. I bumped the tolerance to address that.

This is now ready for review.
dliappis: Should we add a note in `migrate.rst` as well about this change of the default behavior?
pquentin: > Should we add a note in `migrate.rst` as well about this change of the default behavior?

Since this is phrased as a question: I don't think so as it's a bug fix like any other.

Patch:
diff --git a/docs/command_line_reference.rst b/docs/command_line_reference.rst
index 82bf03ea..34d79f19 100644
--- a/docs/command_line_reference.rst
+++ b/docs/command_line_reference.rst
@@ -181,7 +181,7 @@ Selects the track repository that Rally should use to resolve tracks. By default
 ``track-revision``
 ~~~~~~~~~~~~~~~~~~
 
-Selects a specific revision in the track repository. By default, Rally will choose the most appropriate branch on its own but in some cases it is necessary to specify a certain commit. This is mostly needed when testing whether a change in performance has occurred due to a change in the workload. 
+Selects a specific revision in the track repository. By default, Rally will choose the most appropriate branch on its own but in some cases it is necessary to specify a certain commit. This is mostly needed when testing whether a change in performance has occurred due to a change in the workload.
 
 ``track``
 ~~~~~~~~~
@@ -656,7 +656,7 @@ Default value: ``timeout:60`` (applies any time ``timeout`` is not specified)
 Rally recognizes the following client options in addition:
 
 * ``max_connections``: By default, Rally will choose the maximum allowed number of connections automatically (equal to the number of simulated clients but at least 256 connections). With this property it is possible to override that logic but a minimum of 256 is enforced internally.
-* ``enable_cleanup_closed`` (default: ``false``): In some cases, SSL connections might not be properly closed and the number of open connections increases as a result. When this client option is set to ``true``, the Elasticsearch client will check and forcefully close these connections.
+* ``enable_cleanup_closed`` (default: ``true``): In some cases, `Elasticsearch does not properly close SSL connections <https://github.com/elastic/elasticsearch/issues/76642>`_ and the number of open connections increases as a result. When this client option is set to ``true``, the Elasticsearch client will check and forcefully close these connections.
 * ``static_responses``: The path to a JSON file containing path patterns and the corresponding responses. When this value is set to ``true``, Rally will not send requests to Elasticsearch but return static responses as specified by the file. This is useful to diagnose performance issues in Rally itself. See below for a specific example.
 
 **Examples**
diff --git a/esrally/async_connection.py b/esrally/async_connection.py
index 5ce270ae..09b77d2d 100644
--- a/esrally/async_connection.py
+++ b/esrally/async_connection.py
@@ -206,7 +206,7 @@ class AIOHttpConnection(elasticsearch.AIOHttpConnection):
         )
 
         self._trace_configs = [trace_config] if trace_config else None
-        self._enable_cleanup_closed = kwargs.get("enable_cleanup_closed", False)
+        self._enable_cleanup_closed = kwargs.get("enable_cleanup_closed", True)
 
         static_responses = kwargs.get("static_responses")
         self.use_static_responses = static_responses is not None
diff --git a/esrally/chart_generator.py b/esrally/chart_generator.py
index 9a191a09..085f73ab 100644
--- a/esrally/chart_generator.py
+++ b/esrally/chart_generator.py
@@ -868,7 +868,7 @@ class TimeSeriesCharts:
                         "formatter": "number",
                         "id": str(uuid.uuid4()),
                         "line_width": "1",
-                        "metrics": [{"id": str(uuid.uuid4()), "type": "avg", "field": "value.max"}],
+                        "metrics": [{"id": str(uuid.uuid4()), "type": "avg", "field": "value.median"}],
                         "point_size": "3",
                         "seperate_axis": 1,
                         "split_mode": "filters",
@@ -877,7 +877,7 @@ class TimeSeriesCharts:
                         "split_filters": [
                             {
                                 "filter": "ml_processing_time",
-                                "label": "Maximum ML processing time",
+                                "label": "Median ML processing time",
                                 "color": "rgba(0,191,179,1)",
                                 "id": str(uuid.uuid4()),
                             }


Test patch:
diff --git a/tests/client_test.py b/tests/client_test.py
index a5e5eb3d..583bb346 100644
--- a/tests/client_test.py
+++ b/tests/client_test.py
@@ -28,6 +28,7 @@ import pytest
 import urllib3.exceptions
 
 from esrally import client, doc_link, exceptions
+from esrally.async_connection import AIOHttpConnection
 from esrally.utils import console
 from tests import run_async
 
@@ -349,3 +350,15 @@ class TestRestLayer:
         )
         with pytest.raises(exceptions.SystemSetupError, match="Could not connect to cluster via https. Is this an https endpoint?"):
             client.wait_for_rest_layer(es, max_attempts=3)
+
+
+class TestAsyncConnection:
+    @run_async
+    async def test_enable_cleanup_close(self):
+        connection = AIOHttpConnection()
+        # pylint: disable=protected-access
+        assert connection._enable_cleanup_closed is True
+
+        connection = AIOHttpConnection(enable_cleanup_closed=False)
+        # pylint: disable=protected-access
+        assert connection._enable_cleanup_closed is False
diff --git a/tests/driver/runner_test.py b/tests/driver/runner_test.py
index 12c85ebb..e091fe42 100644
--- a/tests/driver/runner_test.py
+++ b/tests/driver/runner_test.py
@@ -5795,13 +5795,13 @@ class TestComposite:
         assert len(timings) == 3
 
         assert timings[0]["operation"] == "initial-call"
-        assert math.isclose(timings[0]["service_time"], 0.1, rel_tol=0.05)
+        assert timings[0]["service_time"] == pytest.approx(0.1, abs=0.1)
 
         assert timings[1]["operation"] == "stream-a"
-        assert math.isclose(timings[1]["service_time"], 0.2, rel_tol=0.05)
+        assert timings[1]["service_time"] == pytest.approx(0.2, abs=0.1)
 
         assert timings[2]["operation"] == "stream-b"
-        assert math.isclose(timings[2]["service_time"], 0.1, rel_tol=0.05)
+        assert timings[2]["service_time"] == pytest.approx(0.1, abs=0.1)
 
         # common properties
         for timing in timings: