perf gating - #185
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a performance gating framework consisting of perf-gating.py to query BigQuery and evaluate release performance against historical baselines, along with a comprehensive test suite in test-perf-gating.py. The reviewer feedback is highly actionable and addresses three key areas: preventing silent passes when no performance data is found for the current release, validating the release version format to mitigate potential SQL injection vulnerabilities, and robustly parsing JSON output from external tools to handle trailing non-JSON warnings.
| failed = False | ||
|
|
||
| print(f"{'Workload':<45} | {'Current':<10} | {'Daily Avg':<10} | {'Diff %':<8} | {'Baselines...'}") | ||
| print("-" * 120) | ||
|
|
||
| for w_key, sources in workloads.items(): | ||
| current = sources.get('current') | ||
| if current is None: | ||
| continue | ||
| daily_avg = sources.get('daily_avg') | ||
|
|
||
| diff_daily_pct = ((current - daily_avg) / daily_avg * 100) if daily_avg else 0 | ||
|
|
||
| baseline_strs = [] | ||
| for b in baselines: | ||
| b_val = sources.get(b) | ||
| if b_val: | ||
| diff_b = ((current - b_val) / b_val * 100) | ||
| baseline_strs.append(f"{b}: {b_val:.2f} ({diff_b:+.2f}%)") | ||
| if diff_b < -threshold: | ||
| print(f"FAILED: {w_key} vs {b} baseline ({diff_b:+.2f}% is worse than -{threshold}%)") | ||
| failed = True | ||
|
|
||
| if daily_avg and diff_daily_pct < -threshold: | ||
| print(f"FAILED: {w_key} vs daily_avg ({diff_daily_pct:+.2f}% is worse than -{threshold}%)") | ||
| failed = True | ||
|
|
||
| daily_str = f"{daily_avg:.2f}" if daily_avg else "N/A" | ||
| diff_str = f"{diff_daily_pct:+.2f}%" if daily_avg else "N/A" | ||
|
|
||
| print(f"{w_key:<45} | {current:<10.2f} | {daily_str:<10} | {diff_str:<8} | {' | '.join(baseline_strs)}") | ||
|
|
||
| print("-" * 120) | ||
| return failed |
There was a problem hiding this comment.
If the BigQuery query returns no data for the 'current' release (e.g., if the performance tests for the current release have not run or failed to upload), the script will silently pass the gate because failed remains False and no comparisons are made.
To prevent silent passes on missing data, the script should track if any workloads were actually evaluated and fail if the current release has no data.
failed = False
evaluated_any = False
print(f"{'Workload':<45} | {'Current':<10} | {'Daily Avg':<10} | {'Diff %':<8} | {'Baselines...'}")
print("-" * 120)
for w_key, sources in workloads.items():
current = sources.get('current')
if current is None:
continue
evaluated_any = True
daily_avg = sources.get('daily_avg')
diff_daily_pct = ((current - daily_avg) / daily_avg * 100) if daily_avg else 0
baseline_strs = []
for b in baselines:
b_val = sources.get(b)
if b_val:
diff_b = ((current - b_val) / b_val * 100)
baseline_strs.append(f"{b}: {b_val:.2f} ({diff_b:+.2f}%)")
if diff_b < -threshold:
print(f"FAILED: {w_key} vs {b} baseline ({diff_b:+.2f}% is worse than -{threshold}%)")
failed = True
if daily_avg and diff_daily_pct < -threshold:
print(f"FAILED: {w_key} vs daily_avg ({diff_daily_pct:+.2f}% is worse than -{threshold}%)")
failed = True
daily_str = f"{daily_avg:.2f}" if daily_avg else "N/A"
diff_str = f"{diff_daily_pct:+.2f}%" if daily_avg else "N/A"
print(f"{w_key:<45} | {current:<10.2f} | {daily_str:<10} | {diff_str:<8} | {' | '.join(baseline_strs)}")
print("-" * 120)
if not evaluated_any:
print("ERROR: No performance data found for the current release version.")
return True
return failed| parser.add_argument("--threshold", type=float, default=10.0, help="Threshold percentage for pass/fail (e.g., 10.0 for +/- 10%)") | ||
| args = parser.parse_args() | ||
|
|
||
| curr_ver = parse_version(args.release_version) |
There was a problem hiding this comment.
The script is vulnerable to SQL injection because args.release_version is directly interpolated into the BigQuery SQL query in build_query without strict validation. If a malformed or malicious version string is passed, it can manipulate the query logic (for example, parse_version will return (0, 0, 0) and treat it as a major release, but the raw string is still interpolated into the SQL).
To prevent SQL injection and ensure correctness, validate that args.release_version strictly matches the expected version format before proceeding.
| curr_ver = parse_version(args.release_version) | |
| curr_ver = parse_version(args.release_version) | |
| if curr_ver == (0, 0, 0) and args.release_version not in ("0.0.0", "v0.0.0"): | |
| print(f"Error: Invalid release version format '{args.release_version}'. Expected format: x.y.z or vx.y.z") | |
| sys.exit(1) |
| cmd = ["bq", "query", "--project_id=gcs-fuse-test-ml", "--use_legacy_sql=false", "--format=json", query] | ||
| try: | ||
| res = subprocess.check_output(cmd, stderr=subprocess.STDOUT) | ||
| return json.loads(res.decode('utf-8')) |
There was a problem hiding this comment.
When reading JSON output from external tools like bq, trailing non-JSON warnings or status messages can sometimes be appended under heavy load or specific environments. Using json.loads directly will fail in these cases. Following the general rules, use json.JSONDecoder().raw_decode to parse the valid JSON prefix and ignore any trailing garbage.
| return json.loads(res.decode('utf-8')) | |
| decoded_res = res.decode('utf-8').strip() | |
| try: | |
| return json.loads(decoded_res) | |
| except json.JSONDecodeError: | |
| return json.JSONDecoder().raw_decode(decoded_res)[0] |
References
- When parsing JSON output from external tools that may append trailing non-JSON warnings or status messages under heavy load, use json.JSONDecoder().raw_decode to parse the valid JSON prefix and ignore trailing garbage, rather than using json.loads which would fail.
d2500b3 to
fece640
Compare
Feature: Automated Performance Regression Gating (
perf-gating.py)This introduces a robust, automated performance gating script designed to integrate into the Louhi release pipeline. It queries historical benchmark data from BigQuery and enforces strict regression bounds (- 10%) on workloads before allowing a release to pass.
Key Capabilities & Logic:
1m/1mand1m/1gfiles usinghttp1&grpc(withdirect=0,numjobs=48)1m/1mand1m/1gfiles usinghttp1&grpc(withdirect=0,numjobs=48)3.9.0): Compares against the last 2 minor releases (e.g.,3.8.0,3.7.0), the latest patch of the previous minor (3.8.x), and a daily continuous rolling average.3.9.2): Ignores daily averages and compares strictly against the last 2 patch releases (e.g.,3.9.1,3.9.0).4.0.0): Automatically detects major version bumps, drops all baselines, and passes cleanly to establish a new release lineage.kokororuns that occurred after the previous minor release was cut, capping the historical lookback to a maximum of 60 days to ensure relevancy.test-perf-gating.py) mocking BigQuery datasets to validate version parsing, fallback logic, time-capping, and SQL generation.