Make exists_set_param macro available to tracks #1012

dliappis · 2020-06-09T14:21:20Z

Frequently it's useful to specify a setting based on the presence of a
track parameter. To avoid repetition this commit makes a Jinja2 macro
called exists_set_param() available for all tracks (if they import the
rally helpers).

Frequently it's useful to specify a setting based on the presence of a track parameter. To avoid repetition this commit makes a Jinja2 macro called `exists_set_param()` available for all tracks (if they import the rally helpers).

danielmitterdorfer

The change looks fine but I left a few comments regarding the docs.

danielmitterdorfer · 2020-06-09T14:49:24Z

docs/adding_tracks.rst

+      }
+    }
+
+Note the lack of a comma after the first setting ``cluster.routing.allocation.node_initial_primaries_recoveries``. This is intentional since the helper with insert it if the parameter exists (this behavior can be changed using ``comma=False``).


Nit: typo: "the helper will insert it"

danielmitterdorfer · 2020-06-09T14:50:09Z

docs/adding_tracks.rst

+
+Note the lack of a comma after the first setting ``cluster.routing.allocation.node_initial_primaries_recoveries``. This is intentional since the helper with insert it if the parameter exists (this behavior can be changed using ``comma=False``).
+
+Assuming we pass ``--track-params="es_snapshot_restore_recovery_max_bytes_per_sec:-1"`` the helper will end up rending the operation as::


typo: "rending" -> "rendering"

danielmitterdorfer · 2020-06-09T14:56:45Z

docs/adding_tracks.rst

+* ``rally.exists_set_param(setting_name, value, default_value=None, comma=True)``: a `macro <https://jinja.pocoo.org/docs/dev/templates/#macros>`_ that you can use to set the value of a track parameter without having to check if it exists.
+
+.. important::
+    To use macros you must declare ``{% import "rally.helpers" as rally with context %}`` at the top of your track; see :ref:`the docs<track_collect_helper>` for more details and `the geonames track <https://github.com/elastic/rally-tracks/blob/b2f86df5f0c18461fdb64dd9ee1fe16bd3653b9d/geonames/track.json#L1>`_ for an example.


nit: maybe we should call the link geonames track, not the geonames track.

danielmitterdorfer · 2020-06-09T14:58:12Z

docs/adding_tracks.rst

@@ -723,6 +725,52 @@ You can find an example in the ``http_logs`` track::

 The data set that is used in the ``http_logs`` track starts on 26-04-1998 but we want to ignore the first few days for this query, so we start on 15-05-1998. The expression ``{{'15-05-1998' | days_ago(now)}}`` yields the difference in days between now and the fixed start date and allows us to benchmark time range queries relative to now with a predetermined data set.

+* ``rally.collect(parts)``: a `macro <https://jinja.pocoo.org/docs/dev/templates/#macros>`_ that you can use to join track fragments. An example can be seen :ref:`above<track_collect_helper>`.


"An example can be seen" sounds a bit stiff. How about: "You can see an example above" or just "See the example above"?

danielmitterdorfer · 2020-06-09T15:02:12Z

docs/adding_tracks.rst

+    }
+
+
+The parameter ``default_value`` controls the value to use for the setting if it's undefined.


it's -> it is?

danielmitterdorfer · 2020-06-09T15:02:44Z

docs/adding_tracks.rst

+
+Example:
+
+Suppose you need an operation that specifies the Elasticsearch transient setting ``indices.recovery.max_bytes_per_sec`` if and only if it's been provided as a track parameter.


I just git grepped and we have a strong tendency towards "it is" rather than "it's" in our docs. :)

dliappis · 2020-06-09T15:13:26Z

Thanks! Addressed your comments in 58db4ef

danielmitterdorfer

Thanks! I left one very minor comment but feel free to address this without another review round. LGTM

danielmitterdorfer · 2020-06-09T15:16:22Z

docs/adding_tracks.rst

 * ``rally.exists_set_param(setting_name, value, default_value=None, comma=True)``: a `macro <https://jinja.pocoo.org/docs/dev/templates/#macros>`_ that you can use to set the value of a track parameter without having to check if it exists.

 .. important::
-    To use macros you must declare ``{% import "rally.helpers" as rally with context %}`` at the top of your track; see :ref:`the docs<track_collect_helper>` for more details and `the geonames track <https://github.com/elastic/rally-tracks/blob/b2f86df5f0c18461fdb64dd9ee1fe16bd3653b9d/geonames/track.json#L1>`_ for an example.
+    To use macros you must declare ``{% import "rally.helpers" as rally with context %}`` at the top of your track; see :ref:`the docs<track_collect_helper>` for more details and `geonames track <https://github.com/elastic/rally-tracks/blob/b2f86df5f0c18461fdb64dd9ee1fe16bd3653b9d/geonames/track.json#L1>`_ for an example.


I think the article "the" was ok there, I just felt a little bit odd in the link text. But I'd still include it in the text outside the link.

Ah ok (I was wondering if we wanna go for less correct grammar/syntax but less words). Fixed.

hub-cap

super small docs nits, tyvm for moving this here.

hub-cap · 2020-06-09T17:17:24Z

docs/adding_tracks.rst

+* ``rally.exists_set_param(setting_name, value, default_value=None, comma=True)``: a `macro <https://jinja.pocoo.org/docs/dev/templates/#macros>`_ that you can use to set the value of a track parameter without having to check if it exists.
+
+.. important::
+    To use macros you must declare ``{% import "rally.helpers" as rally with context %}`` at the top of your track; see :ref:`the docs<track_collect_helper>` for more details and the `geonames track <https://github.com/elastic/rally-tracks/blob/b2f86df5f0c18461fdb64dd9ee1fe16bd3653b9d/geonames/track.json#L1>`_ for an example.


nit, not sure if it even matters, but the space between text and <> is missing

Thanks fixed in 00a9ed6

hub-cap · 2020-06-09T17:19:46Z

docs/adding_tracks.rst

+    }
+
+
+The parameter ``default_value`` controls the value to use for the setting if it is undefined.


Does this mean that not setting the default_value will result in nothing being added? I assume this is the case, as that is sane. Its probably worth clarifying that here.

Thanks fixed in 00a9ed6

dliappis · 2020-06-10T07:00:14Z

/jenkins test this

Make exists_set_param macro available to tracks

fe0b9f7

Frequently it's useful to specify a setting based on the presence of a track parameter. To avoid repetition this commit makes a Jinja2 macro called `exists_set_param()` available for all tracks (if they import the rally helpers).

dliappis added enhancement Improves the status quo :Track Management New operations, changes in the track format, track download changes and the like labels Jun 9, 2020

dliappis added this to the 2.0.1 milestone Jun 9, 2020

dliappis requested review from hub-cap and danielmitterdorfer June 9, 2020 14:21

dliappis self-assigned this Jun 9, 2020

danielmitterdorfer reviewed Jun 9, 2020

View reviewed changes

Address PR comments

58db4ef

dliappis requested a review from danielmitterdorfer June 9, 2020 15:13

danielmitterdorfer approved these changes Jun 9, 2020

View reviewed changes

Add article

b29d729

hub-cap approved these changes Jun 9, 2020

View reviewed changes

Few more doc comments

00a9ed6

dliappis merged commit 8d328de into elastic:master Jun 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make exists_set_param macro available to tracks #1012

Make exists_set_param macro available to tracks #1012

dliappis commented Jun 9, 2020

danielmitterdorfer left a comment

danielmitterdorfer Jun 9, 2020

danielmitterdorfer Jun 9, 2020

danielmitterdorfer Jun 9, 2020

danielmitterdorfer Jun 9, 2020

danielmitterdorfer Jun 9, 2020

danielmitterdorfer Jun 9, 2020

dliappis commented Jun 9, 2020

danielmitterdorfer left a comment

danielmitterdorfer Jun 9, 2020

dliappis Jun 9, 2020

hub-cap left a comment

hub-cap Jun 9, 2020

dliappis Jun 10, 2020

hub-cap Jun 9, 2020

dliappis Jun 10, 2020

dliappis commented Jun 10, 2020


		Note the lack of a comma after the first setting ``cluster.routing.allocation.node_initial_primaries_recoveries``. This is intentional since the helper with insert it if the parameter exists (this behavior can be changed using ``comma=False``).

		Assuming we pass ``--track-params="es_snapshot_restore_recovery_max_bytes_per_sec:-1"`` the helper will end up rending the operation as::

		@@ -723,6 +725,52 @@ You can find an example in the ``http_logs`` track::

		The data set that is used in the ``http_logs`` track starts on 26-04-1998 but we want to ignore the first few days for this query, so we start on 15-05-1998. The expression ``{{'15-05-1998' \| days_ago(now)}}`` yields the difference in days between now and the fixed start date and allows us to benchmark time range queries relative to now with a predetermined data set.

		* ``rally.collect(parts)``: a `macro <https://jinja.pocoo.org/docs/dev/templates/#macros>`_ that you can use to join track fragments. An example can be seen :ref:`above<track_collect_helper>`.

		}


		The parameter ``default_value`` controls the value to use for the setting if it's undefined.


		Example:

		Suppose you need an operation that specifies the Elasticsearch transient setting ``indices.recovery.max_bytes_per_sec`` if and only if it's been provided as a track parameter.

Make exists_set_param macro available to tracks #1012

Make exists_set_param macro available to tracks #1012

Conversation

dliappis commented Jun 9, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented Jun 9, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dliappis commented Jun 10, 2020