Chart generator segment memory and new track combinations #668
Conversation
and also update the release-charts to cover the default benchmarks too.
--chart-flavor should not be used when generating release charts (and shouldn't have a default value).
For nightly benchamarks (presented as Time Series) we need to generate two sets of json files based on the flavor. Thus we need unique titles for the visualizations and dashboard to separate per flavor.
Chart-generator needs to support generation from both normal tracks as well as combinations of tracks. Skip relying on fixed substrings from `--configuration-name` and also do some general refactorings.
esrally/chart_generator.py
Outdated
| @@ -51,11 +53,18 @@ def index_label(race_config): | |||
| return label | |||
|
|
|||
|
|
|||
| def format_title(environment, track_name, suffix=None): | |||
| def format_title(chart_type, environment, track_name, suffix=None, flavor=None): | |||
There was a problem hiding this comment.
Nit: I think the suffix should always be the last parameter.
esrally/chart_generator.py
Outdated
| title = "{}-{}".format(environment, track_name) | ||
|
|
||
| # as we generate two sets of Time Series/Nightly charts per flavor, the titles needs to be different. | ||
| if chart_type == TimeSeriesCharts: |
There was a problem hiding this comment.
This smells as if it is now in the wrong place if we need to do a type test. IMHO we should implement this now as a method on the respective chart classes.
There was a problem hiding this comment.
The problem originally stemed from calling the static method index from within the floating function generate_index_ops:
rally/esrally/chart_generator.py
Line 1341 in c331b00
I've refactored things in 5a3beda
esrally/chart_generator.py
Outdated
| @@ -755,6 +769,9 @@ def index(environment, race_configs, title): | |||
| } | |||
| } | |||
|
|
|||
| def __repr__(self): | |||
esrally/chart_generator.py
Outdated
| @@ -1187,6 +1319,9 @@ def index(environment, race_configs, title): | |||
| } | |||
| } | |||
|
|
|||
| def __repr__(self): | |||
esrally/chart_generator.py
Outdated
| structures = [] | ||
| for flavor, race_configs_per_flavor in race_configs.items(): | ||
| for race_configs_per_track in race_configs_per_flavor: | ||
| logger.debug("Generating charts for race_configs with name:[{}]/flavor:[{}]".format(race_configs_per_track[0].name, flavor)) |
There was a problem hiding this comment.
Our convention is to use plain logger strings for log statements instead of the format API.
esrally/chart_generator.py
Outdated
| idx_race_configs = list(filter(lambda c: "indexing" in c.charts, race_configs)) | ||
| for race_conf in idx_race_configs: | ||
| logger.debug("Gen index visualization for race config with name:[{}] / label:[{}] / flavor: {} / license: {}".format( |
There was a problem hiding this comment.
Our convention is to use plain logger strings for log statements instead of the format API.
esrally/chart_generator.py
Outdated
|
|
||
|
|
||
| def filter_string(environment, race_config): | ||
| def filter_string_time_series(environment, race_config): |
There was a problem hiding this comment.
This should now be a method on the respective classes if we need to do type tests.
esrally/chart_generator.py
Outdated
| return self.configuration.get("flavor") | ||
|
|
||
| @property | ||
| def lic(self): |
There was a problem hiding this comment.
What is lic? License? Is there a reason for the shortcut?
There was a problem hiding this comment.
Yes, it's license, but license is a built-in in Python: https://docs.python.org/3/library/builtins.html
There was a problem hiding this comment.
🤦♂️ - fair enough but does a property name really clash with a builtin? I mean, it's always used in the context of its enclosing object.
There was a problem hiding this comment.
It produces noise on the IDE at well and that was an additional motivation behind it. For variables at least it seems to be a strong antipattern to shadow a built-in and @property in Python makes it act as an instance variable so didn't want to do this.
How about eslicense? Naming's not my strongest suit and yet it's hitting me all the time :)
There was a problem hiding this comment.
Maybe es_license then?
ebadyano
added
the
:internal
Support new format of track combinations file and include new segment memory visualizations.