Skip to content

docs(#1094): unify SLO documentation - #1184

Open
fullsend-ai-coder[bot] wants to merge 4 commits into
mainfrom
agent/1094-unify-slo-docs
Open

fullsend-ai-coder[bot] wants to merge 4 commits into
mainfrom
agent/1094-unify-slo-docs

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Unifies Packit's SLO documentation into the official page docs/service-level-objectives.md. The content of the error-budgets research (SLOs-SLIs.md and users-expectations.md in packit/research) is merged in, so the website has one authoritative page instead of an official page plus a research page.

The page now has:

  • Job outcomes: definitions of success / failure / error
  • Objectives: SLO1–SLO3 with the existing official thresholds. SLO2 keeps "within 5 minutes after the Copr build has finished", not the research's 12 hours. The heading text is unchanged, so existing anchors still resolve.
  • Indicators: the SLIs for each objective and how we measure them (Prometheus Pushgateway, histograms whose buckets match the SLO thresholds, Grafana). I checked these against packit_service/worker/monitoring.py and jobs.py in packit-service.
  • Why these objectives and indicators: each SLO tied back to the user expectations we collected (a quick "accepted" status → SLO1; builds should give a real answer → SLO2, measured from the end of the Copr build because build length isn't under our control; Testing Farm is outside our control → lower SLO3 target)
  • Guidance for other teams: ask users first, start small, measure what you control, use distributions rather than averages, avoid 100%, revisit targets
  • Background: a note that this page supersedes the research, which is kept as historical background

Notes for reviewers

  • Please check SLO1's threshold against current monitoring. The page still says 15 seconds, as before. packit-service's first_initial_status_time histogram has a 15s bucket. However, the no_status_after_25_s counter uses 25s, and the week 47 2022 weekly post said SLO1 was raised from 15s to 30s (packit-service#1776). I did not change the published thresholds because the issue didn't ask for that.
  • Follow-up in another repo: the research page comes from packit/research through make import, so it can't be retired here. research/monitoring/error-budgets/SLOs-SLIs.md in packit/research should be changed to point to this page or marked as historical.

Testing

  • The full site build (make import && yarn build) could not run in the sandbox because git clones fail TLS verification. Instead, I compiled the page with @mdx-js/mdx@1.6.22 (the MDX version Docusaurus 2.4.1 uses). The table, headings and admonition compiled without errors. All new links are external, and the image paths are unchanged. CI's test-deploy job runs the full build.
  • pre-commit couldn't fetch the hook repos for the same TLS reason. I ran check-merge-conflict and detect-private-key (pre-commit-hooks v5.0.0) directly, and both passed. The prettier hook excludes docs/.

🤖 Generated with Claude Code


Closes #1094

Post-script verification

  • Branch is not main/master (agent/1094-unify-slo-docs)
  • Secret scan passed (gitleaks — c0e3687835a9f7786e74d7610b56d2c684879ec9..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

Merge the content of the error-budgets research (SLOs-SLIs.md and
users-expectations.md in packit/research) into the official Service
Level Objectives page so there is a single authoritative document.

The page now contains:
- job-outcome definitions (success / failure / error)
- the three SLOs with the existing official thresholds (SLO2 keeps
  "5 minutes after the Copr build has finished", not the research's
  12 hours)
- the SLIs per objective and how they are measured (Prometheus
  Pushgateway, histograms with buckets aligned to SLO thresholds,
  Grafana), checked against packit-service monitoring code
- a rationale section explaining why each objective and indicator was
  chosen, based on the collected user expectations
- guidance for other teams defining their own SLOs
- a note that the page supersedes the research, which stays as
  historical background

SLO headings keep their text, so existing anchors do not change.

Note: the full site build (make import && yarn build) could not run
in the sandbox because git clones fail on TLS verification. The page
was compiled with @mdx-js/mdx 1.6.22 (the MDX version used by
Docusaurus 2.4.1) instead. pre-commit could not fetch hook repos;
check-merge-conflict and detect-private-key were run directly and
passed; prettier excludes docs/.

Closes #1094

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:18 AM UTC · Completed 8:29 AM UTC

Commit: 1e46fee · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $3.90

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@TomasTomecek TomasTomecek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, I actually like the laid out casual tone

The objectives below talk about jobs (builds and tests) ending with a status.
To make them unambiguous, we distinguish these outcomes:

- **success**: the job finished and everything is awesome

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this made laugh 😂

@fullsend-ai-review fullsend-ai-review Bot added the risk/low PR risk: low label Oct 26, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

<!-- fullsend:risk-assessment -->
Risk Assessment: low (1/5)

Details

This PR changes one documentation file (156 lines, which puts blast radius at medium) and touches no protected, security-sensitive, CI or dependency paths; the author is a bot, the file has had no recent churn or fix history, and the size fits the single-task issue it closes, giving 0.50x1.25 + 0.30x1.57 + 0.20x1.6 = 1.42, rounded to 1.

Previous run

<!-- fullsend:risk-assessment -->
Risk Assessment: moderate (2/5)

Details

This bot-authored PR changes only docs, edits a single file, touches no protected or security-sensitive paths and has no dependency or CI changes, so most signals score 1; the weighted total of 1.53 rounds up to 2 because the 145-line diff counts as a medium blast radius, the target doc was last changed over 180 days ago, and linked issue #1094 has been open for more than 180 days.

Previous run (2)

<!-- fullsend:risk-assessment -->
Risk Assessment: moderate (2/5)

Details

This bot PR changes one docs file and touches no protected, security, CI or dependency paths; it scores 1.53, rounded up to 2 (moderate), because of the medium blast radius of a 130-line change, a file untouched for more than 2.5 years, a linked issue open for more than 180 days, and retirement of the research page deferred to packit/research.

Previous run (3)

<!-- fullsend:risk-assessment -->
Risk Assessment: low (1/5)

Details

Docs-only edit to one file by a known bot (130 lines), with no protected, security, CI or dependency paths, a file with no churn since January 2024, and a linked issue whose scope matches the PR (composite 0.5x1.25 + 0.3x1.57 + 0.2x1.8 = 1.46, rounded to 1).

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Review

Findings

Medium / Low / Info

  • [design-smell] docs/service-level-objectives.md:133: The SLO1 history paragraph (raised to 30s in late 2022 in packit-service#1776, changed back to the first status within 15s in March 2023) is under "Why these objectives and indicators", but it explains neither why SLO1 exists nor why 15s was chosen. The indicator table (line 70) also says the SLO1 "no status" counter uses a 25s limit, not the published 15s. So the official page shows a mismatch between the objective and how it is measured, and the PR body leaves this to maintainers. This needs a maintainer decision, not an automated fix.
    Remediation: Have a maintainer confirm the SLO1 threshold against current monitoring. Then either make the text and the counter agree, or move the history note to the Background section so the rationale section only explains the choices.
  • [complexity-ratio] docs/service-level-objectives.md:95: The "How we measure" section ends with a detailed list (lines 95-110) of how the indicators differ from the objectives. The list comes from packit-service internals: which outcomes reach the build histogram, how Copr infrastructure failures are reported, and when SRPM or newer-build cases skip reporting. These are claims about another repository's code, so they will go out of date whenever packit-service monitoring changes, and nothing here will catch it.
    Remediation: Optional. Consider shortening the list to one sentence saying the histograms only approximate SLO2 and SLO3, and track the detailed gaps in packit-service or packit/research.
  • [provenance-warning] N/A: The prior review context was discarded because provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments, and severity anchoring was skipped for this run.
Previous run

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImVkZ2UtY2FzZSIsImZpbGUiOiJkb2NzL3NlcnZpY2UtbGV2ZWwtb2JqZWN0aXZlcy5tZCIsImlkIjoiZl8xMzVkMzk4NWY1ODI1ZTdmIiwibGluZSI6MTAyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJkb2NzL3NlcnZpY2UtbGV2ZWwtb2JqZWN0aXZlcy5tZCIsImlkIjoiZl80MGRhNjFjNzRiOWY5OTJiIiwibGluZSI6MTI0LCJhY3Rpb25hYmxlIjpmYWxzZX1dfQ== -->

Review

Findings

Medium / Low / Info

  • [edge-case] docs/service-level-objectives.md:102 — The new list of SLO2 approximation limits covers only one direction of the mismatch. It says builds that end with an error on Packit's side are missing from the histogram, which is correct. It leaves out the opposite case. In packit-service, CoprBuildEndHandler._run (packit_service/worker/handlers/copr.py) reports every Copr status other than COPR_API_SUCC_STATE as failure ("RPMs failed to be built."). That includes failures caused by Copr infrastructure. measure_time_after_reporting() then records them in copr_build_end_reported_after_time. By the page's own Job outcomes definitions, these are error outcomes, but the SLO2 histogram counts them as good failures. The handler also skips the measurement when the SRPM build failed or a newer run exists (has_newer_run), even though those builds end with success or failure.
    Remediation: Add a bullet saying that failures on the Copr side, including infrastructure failures, are reported and measured as failures. Also say that superseded builds and builds whose SRPM build failed are not measured.

  • [logic-error] docs/service-level-objectives.md:124 — The page now calls itself "the official definition" (line 14). It also adds a rationale that "the first objective is about the first status only, with a strict time limit" under the 15-second SLO1 heading. That conflicts with this repo's own weekly/2022.md:508, which says SLO1 "was increased from 15s to 30s to account for setting all statuses" (packit-service#1776). Current packit-service monitoring.py records both first_initial_status_time (buckets 5/15/20/25/30) and last_initial_status_time (buckets 5/15/30/60/120). The counter is no_status_after_25_s. The 15-second heading itself predates this PR. However, the new "official" wording and the "first status only" rationale present an unresolved question as settled, and the page doesn't mention the 2022 change or the last-status histogram. The PR body flags this but leaves it open. A maintainer needs to decide the actual SLO1 definition, so this is not left to automated fixing.
    Remediation: Have maintainers confirm SLO1: 15 seconds for the first status, or 30 seconds for all statuses. Then either update the heading and rationale to match, or add a note saying that measuring only the first status replaced the 2022 change to 30 seconds, and whether last_initial_status_time is still an SLO1 indicator.

  • [provenance-warning] — Prior review context was discarded because provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments, and severity anchoring was skipped for this run.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6ImRvY3Mvc2VydmljZS1sZXZlbC1vYmplY3RpdmVzLm1kIiwiaWQiOiJmXzVmNTM5ZjY1MDQwMTQ0ZjIiLCJsaW5lIjo4NiwiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoibG9naWMtZXJyb3IiLCJmaWxlIjoiZG9jcy9zZXJ2aWNlLWxldmVsLW9iamVjdGl2ZXMubWQiLCJpZCI6ImZfN2ZlZjgzNjUzMjhlMjA1NCIsImxpbmUiOjcwLCJhY3Rpb25hYmxlIjp0cnVlfV19 -->

Review

Findings

Low

  • [logic-error] docs/service-level-objectives.md:86: The "How we measure" text says that, because the histogram buckets match the SLO thresholds, we can directly compute the percentage of requests under the threshold. That is true only for the time threshold. The page defines good outcomes as success or failure only (line 28), and SLO2/SLO3 require a success or failure status, but the histograms ignore outcome. In packit-service, test_run_finished_time is also recorded for Testing Farm error/canceled results (handlers/testing_farm.py), so an infrastructure error that finishes within 12h counts as good. copr_build_end_reported_after_time is recorded only on the success and failure paths (handlers/copr.py), so Packit-side errors drop out of both the numerator and the denominator. The bucket ratio alone does not compute SLO2 or SLO3 as defined, which matters for a page meant to guide other teams in deriving SLIs.
    Remediation: Say the histograms give the share of jobs that finished within the time threshold, whatever the outcome. Then explain how error outcomes are accounted for (for example the queued/started/finished counter gaps, or a per-outcome metric), or state plainly that the current SLIs approximate the SLO.

  • [logic-error] docs/service-level-objectives.md:70: The SLO1 indicator "Number of PRs or commits with no status from Packit after 25 seconds", together with the rationale at lines 107-108 ("No status at all ... has dropped the event"), suggests the counter catches events that never got a status. In packit-service, no_status_after_25_s is incremented in push_statuses_metrics only when a first status was eventually set and took more than 25s (jobs.py). The function returns early when there is no status feedback. Dropped events are not counted by either SLO1 indicator.
    Remediation: Reword it to "Number of PRs or commits whose first status took longer than 25 seconds". Do not imply that dropped events are detected, or name the mechanism that does detect them.

Info

  • [provenance-warning] Prior review context was discarded because provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments, and severity anchoring was skipped.

Labels: Docs-only SLO documentation change not tied to a specific integration, matching the area label on linked issue #1094.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (3)

<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6ImRvY3Mvc2VydmljZS1sZXZlbC1vYmplY3RpdmVzLm1kIiwiaWQiOiJmX2NiMjY5ODUzYmE2NjEyN2YiLCJsaW5lIjo3MCwiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiZG9jLXN0eWxlIiwiZmlsZSI6ImRvY3Mvc2VydmljZS1sZXZlbC1vYmplY3RpdmVzLm1kIiwiaWQiOiJmX2UwNjY3ZGUyZjBjYTkxNTciLCJsaW5lIjoxMDEsImFjdGlvbmFibGUiOnRydWV9XX0= -->

Review

Findings

Low

  • [logic-error] docs/service-level-objectives.md:70 — The SLO1 indicator row counts PRs or commits with no status "after the time limit". On this page that limit is SLO1's 15 seconds, and lines 86-87 say the measurements are aligned with the SLO thresholds. But the counter that exists in packit-service is no_status_after_25_s, which uses 25 seconds. The PR body mentions this, but the page does not. The histograms do match the thresholds (15s, 300s, 12h buckets). This counter does not.
    Remediation: State the real threshold, e.g. "Number of PRs or commits with no status from Packit after 25 seconds", or say that this counter uses a looser threshold than the SLO.
  • [doc-style] docs/service-level-objectives.md:101 — The new rationale subheadings (### SLO1: a quick first status, ### SLO2: builds end with a real answer at line 112, ### SLO3: tests with a lower target at line 125) start with a lowercase word after the colon. The Objectives headings on the same page are capitalized (### SLO1: Changes to GitHub PRs ...).
    Remediation: Capitalize the first word after the colon: "A quick first status", "Builds end with a real answer", "Tests with a lower target".

On the reviewer note about SLO1's threshold: git history shows that commit 76caf5b (2023-03-22, "Update SLO1 definition") deliberately changed SLO1 from "a status update within 30 seconds" to "the first status update within 15 seconds". That came after the late-2022 change to 30s for all statuses. So the published 15-second value is the intended current threshold, and this PR is right to keep it.


Labels: The PR only rewrites a documentation page (SLO docs).


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the kind/documentation Improvements to docs label Oct 26, 2026
@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 8:30 AM UTC · Completed 8:34 AM UTC

Commit: 1e46fee · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $0.64

- State the real 25-second limit of the SLO1 "no status" counter in the
  indicators table and note it is looser than the 15-second SLO1 limit.
- Capitalize the first word after the colon in the rationale
  subheadings to match the Objectives headings.

Note: pre-commit could not fetch hook repositories (TLS verification
failure in the sandbox). check-merge-conflict and detect-private-key
(pre-commit-hooks v5.0.0) were run directly and passed; the prettier
hook excludes docs/.

Addresses #1184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Fixed both low-severity findings in docs/service-level-objectives.md. The SLO1 'no status' counter now states its real 25-second limit, and the rationale subheadings are capitalized. The repo has no test or lint suite, and a full site build needs network clones that fail in the sandbox. Instead, the page was compiled with @mdx-js/mdx 1.6.22 (the version Docusaurus 2.4.1 uses) without errors. pre-commit could not fetch its hook repos (TLS failure), so check-merge-conflict and detect-private-key were run directly and passed. The prettier hook excludes docs/. Project CI on the previous head was all passing.

Fixed (3):

  1. [logic-error] SLO1 indicator row says the no-status counter fires 'after the time limit' (implying SLO1's 15s), but the existing packit-service counter uses 25 seconds (docs/service-level-objectives.md): Changed the SLO1 indicator to 'Number of PRs or commits with no status from Packit after 25 seconds (a looser limit than the 15 seconds of SLO1)' and re-padded the table. The paragraph saying histogram buckets match the SLO thresholds is still accurate and was left as is.
  2. [doc-style] Rationale subheadings start with a lowercase word after the colon, unlike the Objectives headings (docs/service-level-objectives.md): Capitalized the rationale subheadings: 'SLO1: A quick first status', 'SLO2: Builds end with a real answer', 'SLO3: Tests with a lower target'. Docusaurus lowercases heading anchors, so the anchors do not change.
  3. Merge conflict check: The forge reports the PR as MERGEABLE (base: main), so no merge or rebase was needed.

Tests: passed

CI inspections (4):

  1. Test deployment (passing, success): Full Docusaurus test build passed on the previous head.
  2. pre-commit.ci - pr (passing, success): pre-commit.ci passed on the previous head.
  3. packit-service/check (passing, success): Zuul check passed.
  4. add-pr-to-the-prs-board (passing, success): Board automation passed.

Conflict update: skipped; strategy none; forge MERGEABLE; target main

Decision points
  • How to describe the 25-second counter (alternatives: Only state 25 seconds, State 25 seconds and note it is looser than the SLO1 limit; rationale: Naming both values keeps readers from assuming that every indicator matches the SLO thresholds exactly, which the reviewer suggested as an acceptable option.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 8:34 AM UTC · Ended 8:45 AM UTC

Commit: 17df112 · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@fullsend-ai-review fullsend-ai-review Bot added risk/moderate PR risk: moderate and removed risk/low PR risk: low labels Oct 26, 2026
fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review fullsend-ai-review Bot added the area/general Not tied to a specific area label Oct 26, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:34 AM UTC · Completed 8:45 AM UTC

Commit: 17df112 · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $4.10

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 8:46 AM UTC · Completed 8:50 AM UTC

Commit: 17df112 · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $0.75

Address the second round of review feedback on PR #1184:

- Describe the SLO1 counter accurately: it counts PRs or commits whose
  first status took longer than 25 seconds, and neither SLO1 indicator
  sees events that never get a status.
- Say that the histograms give the share of jobs that finished within the
  time threshold, whatever the outcome, so they approximate SLO2 and SLO3.
  Explain how error outcomes are handled for test runs (counted) and for
  builds (missing from the histogram).

Verification: pre-commit could not fetch its hook repositories (TLS
verification failure in the sandbox). The prettier hook excludes docs/.
check-merge-conflict and detect-private-key were checked with
equivalent pattern searches and passed. The page compiles with
@mdx-js/mdx 1.6.22.

Addresses #1184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed both low-severity logic-error findings in docs/service-level-objectives.md. The SLO1 counter is now described as counting first statuses that took longer than 25s, and the page notes that dropped events are not captured. The How we measure section now says the histograms measure only time, whatever the outcome, so they approximate SLO2/SLO3, and it explains how error outcomes are handled for test runs and builds. pre-commit could not fetch its hook repos (sandbox TLS failure). The applicable hooks were checked with equivalent pattern searches, and the page compiles with @mdx-js/mdx 1.6.22.

Fixed (3):

  1. [logic-error] docs/service-level-objectives.md:86 - histogram bucket ratio only reflects the time threshold, not the success/failure outcome required by SLO2/SLO3 (docs/service-level-objectives.md): Reworded the How we measure text: the histograms give the percentage of measured jobs that finished within the time threshold, whatever the outcome. Added a paragraph that says plainly the current indicators approximate SLO2 and SLO3. It explains that test-run durations include error and canceled results (so a Testing Farm error within 12h counts as within the threshold), and that the build-reporting duration is recorded only on success or failure (so Packit-side build errors are missing from the histogram). Verified against packit-service handlers/testing_farm.py and handlers/copr.py.
  2. [logic-error] docs/service-level-objectives.md:70 - SLO1 counter wording implies it detects events that never got a status (docs/service-level-objectives.md): Changed the SLO1 indicator to 'Number of PRs or commits whose first status took longer than 25 seconds'. Added a note that both SLO1 indicators are recorded only when Packit sets a first status, so an event dropped without any status shows up in neither. Verified against push_statuses_metrics in packit-service worker/jobs.py, which returns early when no status was set.
  3. Conflict update: forge mergeability check: GitHub reports mergeable=MERGEABLE, so there is no conflict to reconcile. Skipped (no merge or rebase).

Tests: passed

CI inspections (4):

  1. Test deployment (passing, success): Full Docusaurus test build passed on the previous head.
  2. pre-commit.ci - pr (passing, success): pre-commit.ci passed on the previous head.
  3. packit-service/check (passing, success): Zuul check passed.
  4. add-pr-to-the-prs-board (passing, success): Project board automation succeeded.

Conflict update: skipped; strategy none; forge MERGEABLE; target main

Decision points
  • How to account for error outcomes in the SLO2/SLO3 indicator description (alternatives: Claim that the queued/started/finished counter gaps account for errors, State plainly that the time-based indicators approximate the SLOs and describe how errors are handled per metric; rationale: In packit-service, test_runs_finished is incremented for every non-running result, errors included, so counter gaps do not reveal Testing Farm errors. Claiming they do would introduce a new inaccuracy. The reviewer offered the plain-approximation wording as an acceptable remediation.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 8:51 AM UTC · Ended 9:02 AM UTC

Commit: 2bbfccb · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:51 AM UTC · Completed 9:01 AM UTC

Commit: 2bbfccb · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $4.03

@fullsend-ai-coder

fullsend-ai-coder Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:02 AM UTC · Completed 9:06 AM UTC

Commit: 2bbfccb · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $0.85

Address the third round of review feedback on PR #1184:

- List the remaining ways the SLO2 histogram differs from the objective:
  Packit reports every unsuccessful Copr build as a failure, including
  Copr infrastructure failures, so those errors are measured as
  failures. Failed builds are not reported or measured when the SRPM
  build already failed or a newer build for the same target exists.
- Add a note on SLO1 history: the limit was raised to 30 seconds for
  all statuses in late 2022 (packit-service#1776), and in March 2023
  (commit 76caf5b) SLO1 was changed to the first status within 15
  seconds. The published threshold is unchanged; maintainers should
  still confirm it matches current monitoring.

Verification: pre-commit could not fetch its hook repositories (TLS
verification failure in the sandbox), and the hook executables are not
installed. The prettier hook excludes docs/. check-merge-conflict and
detect-private-key were checked with equivalent pattern searches and
passed. The page compiles with @mdx-js/mdx 1.6.22.

Addresses #1184

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 3 (bot-triggered)

Addressed both review findings in docs/service-level-objectives.md. The SLO2 approximation list now says that Copr-side failures, including infrastructure failures, are measured as failures, and that failed builds are not measured when the SRPM build already failed or a newer build exists (checked against packit-service's CoprBuildEndHandler). A history note now explains the 2022 change of SLO1 to 30 seconds and the March 2023 change back to the first status within 15 seconds. A maintainer should still confirm the SLO1 definition and whether last_initial_status_time is an SLO1 indicator. The page compiles with MDX 1.6.22. pre-commit could not fetch its hook repos (sandbox TLS failure), so its checks were reproduced with equivalent pattern searches. All project CI checks pass.

Fixed (2):

  1. [edge-case] docs/service-level-objectives.md:102 - SLO2 approximation list covers only Packit-side errors missing from the histogram, not Copr-side failures counted as good or the builds that are skipped (docs/service-level-objectives.md): Checked against CoprBuildEndHandler._run in packit-service: every Copr status other than success is reported as a failure and measured, and the failure path skips reporting and measurement when the SRPM build already failed or a newer run exists. Added two bullets to the SLO2/SLO3 approximation list. The first says unsuccessful Copr builds, including Copr infrastructure failures, are measured as failures even though the page defines them as errors. The second says failed builds are not reported or measured when their SRPM build already failed or a newer build for the same target exists.
  2. [logic-error] docs/service-level-objectives.md:124 - The page presents the 15-second, first-status-only SLO1 as settled without mentioning the 2022 change to 30 seconds for all statuses (docs/service-level-objectives.md): Took the reviewer's second remediation option and added a short history note under the SLO1 rationale. It says the limit was raised to 30 seconds for all statuses in late 2022 (packit-service#1776) and that in March 2023 SLO1 was changed to the current first-status-within-15-seconds definition. This repo's git history confirms that change (commit 76caf5b, 'Update SLO1 definition'). The published threshold is unchanged. The reviewer marked this finding non-actionable, so a maintainer should still confirm that 15 seconds for the first status is the intended SLO1, and whether last_initial_status_time should count as an SLO1 indicator. The page makes no claim about that metric.

Disagreed (1):

  1. [provenance-warning] Prior review context was discarded because provenance validation failed: This is an informational note about how the review ran. It asks for no change to the PR content, so there is nothing to fix in the code.

Tests: passed

CI inspections (4):

  1. Test deployment (passing, success): The full site build and test deployment passed on the previous head.
  2. pre-commit.ci - pr (passing, success): pre-commit.ci passed.
  3. packit-service/check (passing, success): Zuul check passed.
  4. add-pr-to-the-prs-board (passing, success): Project board automation passed.

Conflict update: skipped; strategy none; forge MERGEABLE; target main

Decision points
  • How to handle the SLO1 threshold finding, which the reviewer said needs a maintainer decision (alternatives: Change the SLO1 heading to 30 seconds for all statuses, Leave the page unchanged and only defer to maintainers, Add a factual history note and keep the published threshold; rationale: Git history shows a maintainer deliberately set the 15-second first-status definition in March 2023, after the 2022 change to 30 seconds. A history note documents that without changing the objective, and the reviewer offered it as a remediation. Maintainers can still change the threshold.)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

🤖 Review · ❌ Terminated · Started 9:07 AM UTC · Ended 9:19 AM UTC

Commit: f9993fd · View workflow run →

@centosinfra-prod-github-app

Copy link
Copy Markdown
Contributor

@fullsend-ai-review fullsend-ai-review Bot added risk/low PR risk: low and removed risk/moderate PR risk: moderate labels Oct 26, 2026
@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself October 26, 2026 09:19

Superseded by updated review

first objective is about the first status only, with a strict time limit and a
high percentage.

In late 2022, the SLO1 limit was raised from 15 to 30 seconds to cover setting

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] design-smell

The SLO1 history paragraph (raised to 30s in late 2022 in packit-service#1776, changed back to the first status within 15s in March 2023) is under 'Why these objectives and indicators', but it explains neither why SLO1 exists nor why 15s was chosen. The indicator table (line 70) also says the SLO1 counter uses a 25s limit, not the published 15s. So the official page shows a mismatch between the objective and how it is measured, and the PR body leaves this to maintainers.

Suggested fix: Have a maintainer confirm the SLO1 threshold against current monitoring. Then either make the text and the counter agree, or move the history note to the Background section so the rationale section only explains the choices.

[what percentage of the measured jobs](https://prometheus.io/docs/practices/histograms/)
finished within the threshold, instead of relying on averages.

The histograms record only the time, whatever the outcome of the job. This means

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] complexity-ratio

The 'How we measure' section ends with a detailed list (lines 95-110) of how the indicators differ from the objectives, taken from packit-service internals: histogram outcomes, how Copr infrastructure failures are reported, and when SRPM or newer-build cases skip reporting. These claims describe another repository's code, so they will go out of date whenever packit-service monitoring changes, and nothing here will catch it.

Suggested fix: Optional: consider shortening the list to one sentence saying the histograms only approximate SLO2 and SLO3, and track the detailed gaps in packit-service or packit/research.

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Oct 26, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:07 AM UTC · Completed 9:19 AM UTC

Commit: f9993fd · View workflow run →

Runtime: claude · Model: opus → claude-opus-5-5 · Effort: high · Cost: $4.30

@TomasTomecek TomasTomecek removed ready-for-review Triggers review agent dispatch ready-for-merge All reviewers approved — ready to merge labels Oct 26, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/general Not tied to a specific area kind/documentation Improvements to docs risk/low PR risk: low

Projects

Development

Successfully merging this pull request may close these issues.

Unify our SLO documentation

2 participants