tvalentyn commented on code in PR #27288:
URL: https://github.com/apache/beam/pull/27288#discussion_r1248021583
##########
sdks/python/apache_beam/testing/analyzers/perf_analysis_test.py:
##########
@@ -193,6 +197,25 @@ def test_alert_on_data_with_reported_change_point(self,
*args):
big_query_metrics_fetcher=None)
self.assertFalse(is_alert)
+ def test_append_anomaly_marker_to_the_right_change_point_in_gh_description(
Review Comment:
```suggestion
def test_change_point_has_anomaly_marker_in_gh_description(
```
##########
sdks/python/apache_beam/testing/analyzers/github_issues_utils.py:
##########
@@ -174,13 +192,16 @@ def get_issue_description(
2 * '\n') if test_description else ''
description += '```' + '\n'
- runs_to_display = [
- _METRIC_INFO_TEMPLATE.format(
- timestamps[i].ctime(), format(metric_values[i], '.2f'))
- for i in reversed(range(min_timestamp_index, max_timestamp_index + 1))
- ]
- runs_to_display[change_point_index - min_timestamp_index] += " <---- Anomaly"
+ runs_to_display = get_runs_data(
+ num_runs_from_change_point=max_results_to_display,
+ metric_values=metric_values,
+ timestamps=timestamps,
+ change_point_index=change_point_index)
+
+ # reverse the list to display the most recent runs first.
+ runs_to_display.reverse()
Review Comment:
- I am not sure adding `get_runs_data` makes sense here, i would probably
keep this within the same function. the name of the helper doesn't say much and
if feels to me that these two functions operate on the same level of
abstraction.
- you can make description as a list (instead of string), and join all
content together at the very end, to avoid extra string concatenations. not a
big deal here but many string concats is generally an antipattern, and you are
going to join more strings later anyway.
- instead of reverting the runs, iterate the list in the reverse order and
append to `description` list.
##########
sdks/python/apache_beam/testing/analyzers/perf_analysis_test.py:
##########
@@ -193,6 +197,25 @@ def test_alert_on_data_with_reported_change_point(self,
*args):
big_query_metrics_fetcher=None)
self.assertFalse(is_alert)
+ def test_append_anomaly_marker_to_the_right_change_point_in_gh_description(
+ self):
+ metric_values, timestamps = get_fake_data_with_change_point()
+ timestamps = [datetime.datetime.fromtimestamp(ts) for ts in timestamps]
+ change_point_index = find_latest_change_point_index(metric_values)
+ description = github_issues_utils.get_runs_data(
+ change_point_index=change_point_index,
+ metric_values=metric_values,
+ timestamps=timestamps,
+ num_runs_from_change_point=(
+ constants._NUM_RESULTS_TO_DISPLAY_ON_ISSUE_DESCRIPTION))
+
+ for i in range(len(description)):
+ if i == change_point_index:
+ cond = constants._ANOMALY_MARKER in description[i]
Review Comment:
if you added helper for test purposes you can still check the final output
instead of helper output. for example, marker occurs only once in the string
,and look for some substring that corresponds to the anomaly value +
changepoint marker
##########
sdks/python/apache_beam/testing/analyzers/perf_analysis_test.py:
##########
@@ -193,6 +197,25 @@ def test_alert_on_data_with_reported_change_point(self,
*args):
big_query_metrics_fetcher=None)
self.assertFalse(is_alert)
+ def test_append_anomaly_marker_to_the_right_change_point_in_gh_description(
Review Comment:
nit: slightly reworded to clarify what is the expected behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
