Create k8s-log-reviewer.md for logging review process#9037
Create k8s-log-reviewer.md for logging review process#9037CatherineF-dev wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CatherineF-dev The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
size/L
pohly
left a comment
pohly
left a comment
There was a problem hiding this comment.
This seems more targeted towards AI than humans? Just wondering.
Is it possible to do some sanity checking that this is actually producing useful reviews?
| `logcheck.conf` (see #133699, #124722, #115669). | ||
|
|
||
| ### 2. Contextual logging: thread a logger, don't proliferate `WithLogger` variants or break API. | ||
| - Prefer adding a `logger`/`ctx` parameter to a *new/unreleased* function rather than a parallel |
There was a problem hiding this comment.
That's typically not possible when converting existing code. Sometimes a new method might be coming anyway, but usually not.
There was a problem hiding this comment.
Updated a little.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: CatherineF-dev The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Stage1: experimental with manual triggering. Sig-instrumentation reviewers can trigger them manually with draft comments. Stage2: update the skill during the review. Stage3: auto-triggering after the skill is mature. Currently, we are at stage1. |
|
@pohly we plan to discuss it in tmr's sig-instrumentation meeting. |
kubernetes-prow
Bot
added
size/M
Add guidelines for SIG-Instrumentation logging review process, including structured and contextual logging standards, review checklist, and testing practices. Update contributors/devel/sig-instrumentation/k8s-log-reviewer.md Co-authored-by: Patrick Ohly <[email protected]> Update contributors/devel/sig-instrumentation/k8s-log-reviewer.md Co-authored-by: Patrick Ohly <[email protected]> Update contributors/devel/sig-instrumentation/k8s-log-reviewer.md Co-authored-by: Patrick Ohly <[email protected]>
CatherineF-dev
force-pushed
the
log-reviewer-2
branch
from
e10b75b to
5c6a312
Compare
| If none apply, report "not a logging change — out of scope" and stop. | ||
|
|
||
| ## Step 3 — Review checklist | ||
| For each finding cite file:line and tag **BLOCKER** / **NIT** / **QUESTION**. |
There was a problem hiding this comment.
Not a blocker for this PR, just a general observation: people dumping a single AI review back into a top-level comment put the work of correlating the finding back to source code on the maintainer developer. It also doesn't enable per-finding discussion threads in the PR review.
If this becomes more common, we may have to make it clear that such reviews are not desirable.
OTOH, they offload work from the bottleneck (maintainer) to the person who presumably has more time (contributor).
🤷
There was a problem hiding this comment.
- AI reviews
As just added here,
Human Review Required: All suggestions, checklists, and reports generated by these agent skills must be reviewed and verified by a human before being posted as comments on any pull request.
- top-level comments
Inline comments are possible.
gh api repos/{owner}/{repo}/pulls/{pr_number}/comments \
-f body="Your comment text here" \
-f commit_id="<commit_sha>" \
-f path="path/to/file.ext" \
-F line=42 \
-f side="RIGHT"
- if a contributor runs an AI review themselves to polish their PR before requesting a maintainer's review
Currently, sig-instrumentation reviewers will choose and post comments after manual review.
There was a problem hiding this comment.
Can gh api also start a review, i.e. keep the comment pending?
The perfect workflow would be:
- Let AI do an initial pass, generating pending comments on findings.
- Maintainer double-checks those pending comments and publishes the review.
There was a problem hiding this comment.
Let AI do an initial pass, generating pending comments on findings.
Maintainer double-checks those pending comments and publishes the review.
Yes, that's the workflow we plan to use since github supports draft/pending comments. We want to evaluate its values first. We will update the skill over time to optimize the review flow.
There was a problem hiding this comment.
@pohly
I created a review tool that it loads related review skills based on context and added draft comment. Then I can accept/edit/reject the comment, then these comments will be posted into github. We might switch review tool to another one, but the review skills keep the same.
Co-authored-by: Patrick Ohly <[email protected]>
Co-authored-by: Patrick Ohly <[email protected]>
3 participants
Add guidelines for SIG-Instrumentation logging review process, including structured and contextual logging standards, review checklist, and testing practices.
Which issue(s) this PR fixes:
Fixes #9036