add deferred orion-report step support for changepoint detection#103
Conversation
mohit-sheth
force-pushed
the
deferred-orion-report-support
branch
from
1da34eb to
5857f20
Compare
mohit-sheth
changed the title
Thread changepoint test names from scan_orion_jsons through to Slack rendering via ProwAnalysisResult.changepoint_tests, eliminating fragile name-guessing between JSON filenames and viz URL keys. Download orion JSONs into per-step subdirectories so both the changepoint set and viz URLs use the same strip_step_prefixes'd folder names. Falls back to flat-glob layout for older jobs. Signed-off-by: Mohit Sheth <msheth@redhat.com>
mohit-sheth
force-pushed
the
deferred-orion-report-support
branch
from
5857f20 to
850605c
Compare
There was a problem hiding this comment.
Pull request overview
Adds support for the upstream “orion-report” consolidated reporting step by updating changepoint JSON discovery, visualization URL construction (single vs multi-link), and Slack rendering, while refactoring shared GCS path utilities.
Changes:
- Add deferred
orion-reporthandling: discover/download per-step Orion artifacts, downloadorion-report-summary.txt, and construct multi-link visualization URLs. - Refactor GCS path and naming helpers into
core.utilsand use a sharedGCSWEB_BASE_URL. - Replace the removed
jsonparser.pychangepoint formatter with new changepoint preview logic inprow_analyzer.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| bugzooka/integrations/slack_fetcher.py | Render multiple viz links (dict) with optional changepoint markers; plumb changepoint_tests through analysis. |
| bugzooka/core/utils.py | Add shared helpers for GCS basename extraction, view URL → GCS path extraction, and step/artifact name normalization. |
| bugzooka/core/constants.py | Introduce GCSWEB_BASE_URL constant used for visualization links. |
| bugzooka/analysis/prow_analyzer.py | Extend analysis result with changepoint_tests; implement deferred-aware orion JSON scanning and summary fallback. |
| bugzooka/analysis/log_summarizer.py | Add orion step discovery, per-step JSON download, report-summary download, and deferred/single viz URL construction. |
| bugzooka/analysis/jsonparser.py | Remove legacy JSON changepoint extraction/formatting helper. |
| README.md | Update repository tree documentation to remove jsonparser.py. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Discover orion step dirs once to avoid redundant GCS listings | ||
| all_step_dirs = list_orion_step_dirs(gcs_path) | ||
| individual_dirs = [(f, p) for f, p in all_step_dirs if "orion-report" not in f] | ||
| download_prow_orion_jsons(individual_dirs, orion_dir) | ||
| download_prow_orion_report_summary(all_step_dirs, log_dir) |
There was a problem hiding this comment.
download_prow_logs() filters out step directories containing orion-report when downloading JSONs, but scan_orion_jsons() has a deferred-mode fallback that expects junit_*.json / output_*.json to be available locally. With the current implementation, JSONs that only exist under the orion-report step’s artifacts will never be downloaded, so changepoint detection will silently fail. Consider also downloading the deferred JSON artifacts from the orion-report step (either into the job root to match the fallback glob, or into a dedicated directory that scan_orion_jsons() checks).
There was a problem hiding this comment.
individual steps have their own jsons, this is not required
| preview_results, changepoint_tests = _collect_changepoints(json_pairs) | ||
|
|
||
| if not changepoint_tests: | ||
| return [], [], set() | ||
|
|
||
| # Full results: use orion report summary if available | ||
| report_file = Path(directory_path) / "orion-report-summary.txt" | ||
| if report_file.exists(): |
There was a problem hiding this comment.
scan_orion_jsons() returns early when changepoint_tests is empty (from JSON parsing), which prevents using orion-report-summary.txt even if it exists. This contradicts the docstring (“uses orion-report-summary.txt if available”) and means deferred/report-only runs can produce no orion output despite having a summary file. Move the report-summary read before the early return, and/or derive preview / changepoint_tests from the summary when JSONs aren’t present.
| def extract_gcs_path(view_url): | ||
| """ | ||
| Extract the GCS path portion from a Prow view URL. | ||
|
|
||
| :param view_url: e.g. https://prow.ci.../view/gs/bucket/path/123 | ||
| :return: 'bucket/path/123' | ||
| """ | ||
| return view_url.split("view/gs/")[1] |
There was a problem hiding this comment.
extract_gcs_path() uses view_url.split('view/gs/')[1], which raises IndexError if the substring isn’t present and will include any querystring/fragment in the returned path. Since this helper is now reused in multiple places, it should validate the input and raise a clear ValueError, and strip ?… / #… from the extracted GCS path.
There was a problem hiding this comment.
Value error is good but "and strip ?… / #… from the extracted GCS path" is not required
There was a problem hiding this comment.
Not sure if copilot listens to the comments like coderabbit.
| changepoint_tests = set() | ||
| for test_name, json_file in json_pairs: | ||
| try: | ||
| with open(json_file, "r") as f: |
There was a problem hiding this comment.
When reading orion JSON files, open(json_file, 'r') relies on the process default encoding. Elsewhere in this module you explicitly use UTF-8; it would be more robust/consistent to open these JSON files with encoding='utf-8' as well.
| with open(json_file, "r") as f: | |
| with open(json_file, "r", encoding="utf-8") as f: |
| if isinstance(viz_url, dict): | ||
| for test_name, url in viz_url.items(): | ||
| if changepoint_tests is not None: | ||
| marker = ":warning:" if test_name in changepoint_tests else ":white_check_mark:" | ||
| else: | ||
| marker = ":chart_with_upwards_trend:" | ||
| header_text += f"{marker} <{url}|{test_name}>\n" | ||
| elif viz_url: |
There was a problem hiding this comment.
New behavior: when viz_url is a dict, _send_error_logs_preview() renders multiple Slack links and (optionally) uses changepoint_tests to choose markers. The existing tests/test_slack_fetcher.py assertions only validate the presence of the “Error Logs Preview” header and don’t cover this multi-link rendering, so regressions in link formatting/markers won’t be caught. Add/extend a test to pass a dict viz_url (and a changepoint_tests set) and assert the rendered header contains the expected link lines.
Support the upstream change where orion failures are reported through a consolidated orion-report step instead of individual orion steps. Adds fallback JSON discovery, scaffolding for per-test viz URL mapping, and multi-link Slack rendering. Refactors shared GCS folder-walking into a common helper.