Skip to content

perf gating - #185

Open
PranjalC100 wants to merge 1 commit into
mainfrom
perf-gating
Open

perf gating#185
PranjalC100 wants to merge 1 commit into
mainfrom
perf-gating

Conversation

@PranjalC100

Copy link
Copy Markdown
Member

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:

  • Strict Workload Targeting: Exclusively filters and evaluates 8 critical configurations:
    • Writes: 1m/1m and 1m/1g files using http1 & grpc (with direct=0, numjobs=48)
    • Reads: 1m/1m and 1m/1g files using http1 & grpc (with direct=0, numjobs=48)
  • Dynamic Baseline Resolution: Intelligently determines what to compare against based on the type of release:
    • Minor Releases (e.g., 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.
    • Patch Releases (e.g., 3.9.2): Ignores daily averages and compares strictly against the last 2 patch releases (e.g., 3.9.1, 3.9.0).
    • Major Releases (e.g., 4.0.0): Automatically detects major version bumps, drops all baselines, and passes cleanly to establish a new release lineage.
  • Rolling Daily Average with Sane Limits: For minor releases, it fetches the daily continuous kokoro runs that occurred after the previous minor release was cut, capping the historical lookback to a maximum of 60 days to ensure relevancy.
  • Regression-Only Failures: The script only fails the pipeline if performance degrades beyond the threshold (e.g., slower by >10%). Unexpected performance speedups will safely pass the gate.
  • Comprehensive Test Coverage: Supported by a fully exhaustive suite of unit tests (test-perf-gating.py) mocking BigQuery datasets to validate version parsing, fallback logic, time-capping, and SQL generation.
@PranjalC100 PranjalC100 changed the title initial version perf gating Aug 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +153 to +186
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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-high high

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.

Suggested change
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)
Comment thread perf-gating.py Outdated
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'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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
  1. 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant