From dbf60b7e99e7c3da91e7f9cbf50cf2c257e2ad0f Mon Sep 17 00:00:00 2001 From: Matt Fleming Date: Mon, 18 Mar 2024 10:34:46 +0000 Subject: [PATCH 1/2] Add --ignore-direction to regressions subcommand Sometimes users want to know about *any* performance change between a PR and a baseline branch, i.e. not just whether there's a regression but also if there's a performance improvement. (cherry picked from commit 116eb5a54572846adca1861e1369930330311d2c) --- otava/main.py | 26 +++++++++++++++++++++++--- tests/cli_help_test.py | 20 ++++++++++++-------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/otava/main.py b/otava/main.py index 09e76085..9d8dbd30 100644 --- a/otava/main.py +++ b/otava/main.py @@ -257,7 +257,11 @@ def update_bigquery(self, test: BigQueryTestConfig, series: AnalyzedSeries): bigquery.insert_change_point(test, metric_name, attributes, cp) def regressions( - self, test: TestConfig, selector: DataSelector, options: AnalysisOptions + self, + test: TestConfig, + selector: DataSelector, + options: AnalysisOptions, + ignore_direction: bool = False, ) -> bool: importer = self.__importers.get(test) @@ -306,7 +310,12 @@ def regressions( m1 = stats.mean_1 m2 = stats.mean_2 change_percent = stats.forward_rel_change() * 100.0 - if m2 * direction < m1 * direction and stats.pvalue < options.max_pvalue: + if ignore_direction: + mean_diff = m2 != m1 + else: + mean_diff = m2 * direction < m1 * direction + + if mean_diff and stats.pvalue < options.max_pvalue: regressions.append( " {:16}: {:#8.3g} --> {:#8.3g} ({:+6.1f}%)".format( metric_name, m1, m2, change_percent @@ -594,6 +603,12 @@ def create_otava_cli_parser() -> argparse.ArgumentParser: regressions_parser.add_argument( "tests", help="name of the test or group of the tests", nargs="+" ) + regressions_parser.add_argument( + "--ignore-direction", + help="ignore the direction of the change in performance", + dest="ignore_direction", + action="store_true", + ) config.add_service_option_groups(regressions_parser) setup_data_selector_parser(regressions_parser) setup_analysis_options_parser(regressions_parser) @@ -695,7 +710,12 @@ def script_main(conf: Config = None, args: List[str] = None): errors = 0 for test in tests: try: - regressions = otava.regressions(test, selector=data_selector, options=options) + regressions = otava.regressions( + test, + selector=data_selector, + options=options, + ignore_direction=args.ignore_direction, + ) if regressions: regressing_test_count += 1 except OtavaError as err: diff --git a/tests/cli_help_test.py b/tests/cli_help_test.py index a8a02559..11f89346 100644 --- a/tests/cli_help_test.py +++ b/tests/cli_help_test.py @@ -341,10 +341,11 @@ def test_otava_regressions_help_output(): # Python 3.13+ formats usage lines and option aliases differently if IS_PYTHON_313_PLUS: usage_and_options = """\ -usage: otava regressions [-h] [--graphite-url GRAPHITE_URL] [--grafana-url GRAFANA_URL] - [--grafana-user GRAFANA_USER] [--grafana-password GRAFANA_PASSWORD] - [--slack-token SLACK_TOKEN] [--postgres-hostname POSTGRES_HOSTNAME] - [--postgres-port POSTGRES_PORT] [--postgres-username POSTGRES_USERNAME] +usage: otava regressions [-h] [--ignore-direction] [--graphite-url GRAPHITE_URL] + [--grafana-url GRAFANA_URL] [--grafana-user GRAFANA_USER] + [--grafana-password GRAFANA_PASSWORD] [--slack-token SLACK_TOKEN] + [--postgres-hostname POSTGRES_HOSTNAME] [--postgres-port POSTGRES_PORT] + [--postgres-username POSTGRES_USERNAME] [--postgres-password POSTGRES_PASSWORD] [--postgres-database POSTGRES_DATABASE] [--bigquery-project-id BIGQUERY_PROJECT_ID] @@ -362,6 +363,7 @@ def test_otava_regressions_help_output(): options: -h, --help show this help message and exit + --ignore-direction ignore the direction of the change in performance --branch [STRING] name of the branch --metrics LIST a comma-separated list of metrics to analyze --attrs LIST a comma-separated list of attribute names associated with the runs (e.g. @@ -385,10 +387,11 @@ def test_otava_regressions_help_output(): -M, --magnitude MAGNITUDE""" else: usage_and_options = """\ -usage: otava regressions [-h] [--graphite-url GRAPHITE_URL] [--grafana-url GRAFANA_URL] - [--grafana-user GRAFANA_USER] [--grafana-password GRAFANA_PASSWORD] - [--slack-token SLACK_TOKEN] [--postgres-hostname POSTGRES_HOSTNAME] - [--postgres-port POSTGRES_PORT] [--postgres-username POSTGRES_USERNAME] +usage: otava regressions [-h] [--ignore-direction] [--graphite-url GRAPHITE_URL] + [--grafana-url GRAFANA_URL] [--grafana-user GRAFANA_USER] + [--grafana-password GRAFANA_PASSWORD] [--slack-token SLACK_TOKEN] + [--postgres-hostname POSTGRES_HOSTNAME] [--postgres-port POSTGRES_PORT] + [--postgres-username POSTGRES_USERNAME] [--postgres-password POSTGRES_PASSWORD] [--postgres-database POSTGRES_DATABASE] [--bigquery-project-id BIGQUERY_PROJECT_ID] @@ -406,6 +409,7 @@ def test_otava_regressions_help_output(): options: -h, --help show this help message and exit + --ignore-direction ignore the direction of the change in performance --branch [STRING] name of the branch --metrics LIST a comma-separated list of metrics to analyze --attrs LIST a comma-separated list of attribute names associated with the runs (e.g. From 98ed1f552ac1777f28ba4a8317f817bbc81b29cb Mon Sep 17 00:00:00 2001 From: Matt Fleming Date: Mon, 18 Mar 2024 11:58:08 +0000 Subject: [PATCH 2/2] Make regressions subcommand support --output param It's useful to get the output of "hunter regressions" in JSON format for post-processing. The default behaviour if the user doesn't pass --output should be the same as before this change. (cherry picked from commit 3400a3cfd3cb5ac044ad697a9c95afbeeda058e8) --- otava/main.py | 53 ++++++++++++------------ otava/report.py | 50 +++++++++++++++++++++-- tests/cli_help_test.py | 33 +++++++-------- tests/report_test.py | 93 ++++++++++++++++++++++++++++-------------- 4 files changed, 152 insertions(+), 77 deletions(-) diff --git a/otava/main.py b/otava/main.py index 9d8dbd30..4f6039c5 100644 --- a/otava/main.py +++ b/otava/main.py @@ -35,7 +35,7 @@ from otava.graphite import GraphiteError from otava.importer import DataImportError, Importers from otava.postgres import Postgres, PostgresError -from otava.report import Report, ReportType +from otava.report import ChangePointReport, RegressionsReport, ReportType from otava.series import AnalysisOptions, AnalyzedSeries, compare from otava.slack import NotificationError, SlackNotifier from otava.test_config import ( @@ -123,7 +123,7 @@ def analyze( series = importer.fetch_data(test, selector) analyzed_series = series.analyze(options) change_points = analyzed_series.change_points_by_time - report = Report(series, change_points) + report = ChangePointReport(series, change_points) produced_report = report.produce_report(test.name, report_type) print(produced_report) return analyzed_series @@ -261,6 +261,7 @@ def regressions( test: TestConfig, selector: DataSelector, options: AnalysisOptions, + report_type: ReportType, ignore_direction: bool = False, ) -> bool: importer = self.__importers.get(test) @@ -309,25 +310,17 @@ def regressions( direction = baseline_series.metric(metric_name).direction m1 = stats.mean_1 m2 = stats.mean_2 - change_percent = stats.forward_rel_change() * 100.0 if ignore_direction: mean_diff = m2 != m1 else: mean_diff = m2 * direction < m1 * direction if mean_diff and stats.pvalue < options.max_pvalue: - regressions.append( - " {:16}: {:#8.3g} --> {:#8.3g} ({:+6.1f}%)".format( - metric_name, m1, m2, change_percent - ) - ) + regressions.append((metric_name, stats)) - if regressions: - print(f"{test.name}:") - for r in regressions: - print(r) - else: - print(f"{test.name}: OK") + report = RegressionsReport(regressions) + produced_report = report.produce_report(test.name, report_type) + print(produced_report) return len(regressions) > 0 def __maybe_create_slack_notifier(self): @@ -508,6 +501,14 @@ def setup_analysis_options_parser(parser: argparse.ArgumentParser): help="use the original edivisive algorithm with no windowing " "and weak change points analysis improvements", ) + parser.add_argument( + "--output", + help="Output format for the generated report.", + choices=list(ReportType), + dest="report_type", + default=ReportType.LOG, + type=ReportType, + ) def analysis_options_from_args(args: argparse.Namespace) -> AnalysisOptions: @@ -585,14 +586,6 @@ def create_otava_cli_parser() -> argparse.ArgumentParser: metavar="DATE", dest="cph_report_since", ) - analyze_parser.add_argument( - "--output", - help="Output format for the generated report.", - choices=list(ReportType), - dest="report_type", - default=ReportType.LOG, - type=ReportType, - ) setup_data_selector_parser(analyze_parser) setup_analysis_options_parser(analyze_parser) @@ -715,6 +708,7 @@ def script_main(conf: Config = None, args: List[str] = None): selector=data_selector, options=options, ignore_direction=args.ignore_direction, + report_type=args.report_type, ) if regressions: regressing_test_count += 1 @@ -724,12 +718,15 @@ def script_main(conf: Config = None, args: List[str] = None): except DataImportError as err: logging.error(err.message) errors += 1 - if regressing_test_count == 0: - print("No regressions found!") - elif regressing_test_count == 1: - print("Regressions in 1 test found") - else: - print(f"Regressions in {regressing_test_count} tests found") + + if args.report_type == ReportType.LOG: + if regressing_test_count == 0: + print("No regressions found!") + elif regressing_test_count == 1: + print("Regressions in 1 test found") + else: + print(f"Regressions in {regressing_test_count} tests found") + if errors > 0: print("Some tests were skipped due to import / analyze errors. Consult error log.") diff --git a/otava/report.py b/otava/report.py index d9cf3c43..94fbb1d4 100644 --- a/otava/report.py +++ b/otava/report.py @@ -15,12 +15,13 @@ # specific language governing permissions and limitations # under the License. -from collections import OrderedDict +from collections import OrderedDict, defaultdict from enum import Enum, unique -from typing import List +from typing import List, Tuple from tabulate import tabulate +from otava.analysis import TTestStats from otava.series import ChangePointGroup, Series from otava.util import format_timestamp, insert_multiple, remove_common_prefix @@ -35,7 +36,7 @@ def __str__(self): return self.value -class Report: +class ChangePointReport: __series: Series __change_points: List[ChangePointGroup] @@ -133,3 +134,46 @@ def __format_regressions_only(self, test_name: str) -> str: return f"Regressions in {test_name}:" + "\n" + "\n".join(output) else: return f"No regressions found in {test_name}." + + +class RegressionsReport: + def __init__(self, regressions: List[Tuple[str, TTestStats]]): + self.__regressions = regressions + + def produce_report(self, test_name: str, report_type: ReportType): + if report_type == ReportType.LOG: + return self.__format_log(test_name) + elif report_type == ReportType.JSON: + return self.__format_json(test_name) + else: + from hunter.main import HunterError + + raise HunterError(f"Unknown report type: {report_type}") + + def __format_log(self, test_name: str) -> str: + if not self.__regressions: + return f"{test_name}: OK" + + output = f"{test_name}:" + for metric_name, stats in self.__regressions: + m1 = stats.mean_1 + m2 = stats.mean_2 + change_percent = stats.forward_change_percent() + output += "\n {:16}: {:#8.3g} --> {:#8.3g} ({:+6.1f}%)".format( + metric_name, m1, m2, change_percent + ) + return output + + def __format_json(self, test_name: str) -> str: + import json + + if not self.__regressions: + return json.dumps({test_name: []}) + + output = defaultdict(list) + for metric_name, stats in self.__regressions: + obj = {"metric": metric_name} + obj.update(stats.to_json()) + output[test_name].append(obj) + + return json.dumps(output) diff --git a/tests/cli_help_test.py b/tests/cli_help_test.py index 11f89346..db49bd01 100644 --- a/tests/cli_help_test.py +++ b/tests/cli_help_test.py @@ -163,11 +163,11 @@ def test_otava_analyze_help_output(): [--bigquery-credentials BIGQUERY_CREDENTIALS] [--update-grafana] [--update-postgres] [--update-bigquery] [--notify-slack NOTIFY_SLACK [NOTIFY_SLACK ...]] [--cph-report-since DATE] - [--output {log,json,regressions_only}] [--branch [STRING]] [--metrics LIST] - [--attrs LIST] [--since-commit STRING | --since-version STRING | - --since DATE] [--until-commit STRING | --until-version STRING | --until DATE] - [--last COUNT] [-P, --p-value PVALUE] [-M MAGNITUDE] [--window WINDOW] - [--orig-edivisive ORIG_EDIVISIVE] + [--branch [STRING]] [--metrics LIST] [--attrs LIST] [--since-commit STRING | + --since-version STRING | --since DATE] [--until-commit STRING | + --until-version STRING | --until DATE] [--last COUNT] [-P, --p-value PVALUE] + [-M MAGNITUDE] [--window WINDOW] [--orig-edivisive ORIG_EDIVISIVE] + [--output {log,json,regressions_only}] tests [tests ...] positional arguments: @@ -184,8 +184,6 @@ def test_otava_analyze_help_output(): --cph-report-since DATE Sets a limit on the date range of the Change Point History reported to Slack. Same syntax as --since. - --output {log,json,regressions_only} - Output format for the generated report. --branch [STRING] name of the branch --metrics LIST a comma-separated list of metrics to analyze --attrs LIST a comma-separated list of attribute names associated with the runs (e.g. @@ -220,12 +218,11 @@ def test_otava_analyze_help_output(): [--bigquery-credentials BIGQUERY_CREDENTIALS] [--update-grafana] [--update-postgres] [--update-bigquery] [--notify-slack NOTIFY_SLACK [NOTIFY_SLACK ...]] [--cph-report-since DATE] - [--output {log,json,regressions_only}] [--branch [STRING]] [--metrics LIST] - [--attrs LIST] + [--branch [STRING]] [--metrics LIST] [--attrs LIST] [--since-commit STRING | --since-version STRING | --since DATE] [--until-commit STRING | --until-version STRING | --until DATE] [--last COUNT] [-P, --p-value PVALUE] [-M MAGNITUDE] [--window WINDOW] - [--orig-edivisive ORIG_EDIVISIVE] + [--orig-edivisive ORIG_EDIVISIVE] [--output {log,json,regressions_only}] tests [tests ...] positional arguments: @@ -242,8 +239,6 @@ def test_otava_analyze_help_output(): --cph-report-since DATE Sets a limit on the date range of the Change Point History reported to Slack. Same syntax as --since. - --output {log,json,regressions_only} - Output format for the generated report. --branch [STRING] name of the branch --metrics LIST a comma-separated list of metrics to analyze --attrs LIST a comma-separated list of attribute names associated with the runs (e.g. @@ -268,7 +263,8 @@ def test_otava_analyze_help_output(): assert ( result.stdout - == usage_and_options + """ + == usage_and_options + + """ minimum accepted magnitude of a change-point computed as abs(new_mean / old_mean - 1.0); use it to filter out stupidly small changes like < 0.01 --window WINDOW the number of data points analyzed at once; the window size affects the @@ -279,6 +275,8 @@ def test_otava_analyze_help_output(): --orig-edivisive ORIG_EDIVISIVE use the original edivisive algorithm with no windowing and weak change points analysis improvements + --output {log,json,regressions_only} + Output format for the generated report. Graphite Options: Options for Graphite configuration @@ -355,7 +353,7 @@ def test_otava_regressions_help_output(): --since-version STRING | --since DATE] [--until-commit STRING | --until-version STRING | --until DATE] [--last COUNT] [-P, --p-value PVALUE] [-M MAGNITUDE] [--window WINDOW] - [--orig-edivisive ORIG_EDIVISIVE] + [--orig-edivisive ORIG_EDIVISIVE] [--output {log,json,regressions_only}] tests [tests ...] positional arguments: @@ -401,7 +399,7 @@ def test_otava_regressions_help_output(): [--since-commit STRING | --since-version STRING | --since DATE] [--until-commit STRING | --until-version STRING | --until DATE] [--last COUNT] [-P, --p-value PVALUE] [-M MAGNITUDE] [--window WINDOW] - [--orig-edivisive ORIG_EDIVISIVE] + [--orig-edivisive ORIG_EDIVISIVE] [--output {log,json,regressions_only}] tests [tests ...] positional arguments: @@ -434,7 +432,8 @@ def test_otava_regressions_help_output(): assert ( result.stdout - == usage_and_options + """ + == usage_and_options + + """ minimum accepted magnitude of a change-point computed as abs(new_mean / old_mean - 1.0); use it to filter out stupidly small changes like < 0.01 --window WINDOW the number of data points analyzed at once; the window size affects the @@ -445,6 +444,8 @@ def test_otava_regressions_help_output(): --orig-edivisive ORIG_EDIVISIVE use the original edivisive algorithm with no windowing and weak change points analysis improvements + --output {log,json,regressions_only} + Output format for the generated report. Graphite Options: Options for Graphite configuration diff --git a/tests/report_test.py b/tests/report_test.py index 3692a799..504bdad3 100644 --- a/tests/report_test.py +++ b/tests/report_test.py @@ -19,7 +19,8 @@ import pytest -from otava.report import Report, ReportType +from otava.analysis import TTestStats +from otava.report import ChangePointReport, RegressionsReport, ReportType from otava.series import Metric, Series @@ -44,12 +45,12 @@ def change_points(series): @pytest.fixture(scope="module") -def report(series, change_points): - return Report(series, change_points) +def cp_report(series, change_points): + return ChangePointReport(series, change_points) def test_report(series, change_points): - report = Report(series, change_points) + report = ChangePointReport(series, change_points) output = report.produce_report("test", ReportType.LOG) assert "series1" in output assert "series2" in output @@ -66,32 +67,64 @@ def test_report(series, change_points): assert len(output.split("\n")) == len(series.time) + 2 + 3 * len(change_points) -def test_json_report(report): - output = report.produce_report("test_name_from_config", ReportType.JSON) +def test_json_report(cp_report): + output = cp_report.produce_report("test_name_from_config", ReportType.JSON) obj = json.loads(output) - expected = {'test_name_from_config': [{'attributes': {}, - 'changes': [{'forward_change_percent': '-11', - 'index': 4, - 'magnitude': '0.124108', - 'mean_after': '1.801429', - 'mean_before': '2.025000', - 'metric': 'series2', - 'pvalue': '0.000000', - 'stddev_after': '0.026954', - 'stddev_before': '0.011180', - 'time': 4}], - 'time': 4}, - {'attributes': {}, - 'changes': [{'forward_change_percent': '-49', - 'index': 6, - 'magnitude': '0.977513', - 'mean_after': '0.504000', - 'mean_before': '0.996667', - 'metric': 'series1', - 'pvalue': '0.000000', - 'stddev_after': '0.025768', - 'stddev_before': '0.067495', - 'time': 6}], - 'time': 6}]} + expected = { + "test_name_from_config": [ + { + "attributes": {}, + "changes": [ + { + "forward_change_percent": "-11", + "index": 4, + "magnitude": "0.124108", + "mean_after": "1.801429", + "mean_before": "2.025000", + "metric": "series2", + "pvalue": "0.000000", + "stddev_after": "0.026954", + "stddev_before": "0.011180", + "time": 4, + } + ], + "time": 4, + }, + { + "attributes": {}, + "changes": [ + { + "forward_change_percent": "-49", + "index": 6, + "magnitude": "0.977513", + "mean_after": "0.504000", + "mean_before": "0.996667", + "metric": "series1", + "pvalue": "0.000000", + "stddev_after": "0.025768", + "stddev_before": "0.067495", + "time": 6, + } + ], + "time": 6, + }, + ] + } assert isinstance(obj, dict) assert obj == expected + + +def test_regressions_report(): + stats = TTestStats(0.5, 1, 2, 3, 4) + report = RegressionsReport([("metric1", stats)]) + output = report.produce_report("test_name", ReportType.LOG) + assert "test_name" in output + assert "metric1" in output + assert "(+100.0%)" in output + + output = report.produce_report("test_name", ReportType.JSON) + obj = json.loads(output) + assert "test_name" in obj + assert len(obj["test_name"]) == 1 + assert obj["test_name"][0]["metric"] == "metric1" + assert float(obj["test_name"][0]["forward_change_percent"]) == 100.0