Skip to content

Create k8s-log-reviewer.md for logging review process #9037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CatherineF-dev wants to merge 4 commits into
base: main
Choose a base branch
from
+102 −0
Open

Create k8s-log-reviewer.md for logging review process #9037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
31df1c2
Create k8s-log-reviewer and k8s-log-instrumenter skills
CatherineF-dev Jun 19, 2026
5c6a312
Refactor folder structure to align with https://agentskills.io/specif…
CatherineF-dev Jun 29, 2026
c19e6c2
Update contributors/devel/sig-instrumentation/k8s-log-reviewer/SKILL.md
CatherineF-dev Jul 2, 2026
c5172fc
Update contributors/devel/sig-instrumentation/k8s-log-reviewer/SKILL.md
CatherineF-dev Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...tors/devel/sig-instrumentation/logging.md → ...rumentation/k8s-log-instrumenter/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
---
name: k8s-log-instrumenter
description: Update a kubernetes/kubernetes pull request that changes kubernetes log code. Use when asked to write a k8s PR/diff that touches logs. Trigger phrases: "write this log in the PR".
---


## Logging

This document provides an overview of the recommended way to develop and implement
Expand Down
96 changes: 96 additions & 0 deletions contributors/devel/sig-instrumentation/k8s-log-reviewer/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
---
name: k8s-log-reviewer
description: Review a kubernetes/kubernetes pull request that changes logging code, applying SIG-Instrumentation approver standards for structured logging (klog InfoS/ErrorS), contextual logging (logr via context, klog.FromContext, WithLogger variants), logcheck enforcement, key/value and verbosity conventions, and ktesting. Use when asked to review a k8s PR/diff that touches logging, klog, structured/contextual logging, or logcheck.conf. Trigger phrases: "review this logging PR", "sig-instrumentation log review", "is this structured/contextual logging correct".
---

# SIG-Instrumentation Logging Review

Review a `kubernetes/kubernetes` PR the way a **SIG-Instrumentation approver** would for **logging**
changes.

This skill reviews **only** logging-relevant changes. If the PR doesn't touch logging, say so and stop.
(For metrics, use `sig-instrumentation-metrics-review` instead.)

## Step 1 — Get the change
- A PR number/URL → `gh pr view <n> --repo kubernetes/kubernetes` + `gh pr diff <n> --repo kubernetes/kubernetes`.
- A local branch → `git diff` against the merge base. A pasted diff → use directly.
- No `gh`/auth → GitHub API (`/repos/kubernetes/kubernetes/pulls/<n>/files`) or ask for the diff.

## Step 2 — Scope to logging
In-scope signals (any of):
- Adds/changes `klog.*` calls (`InfoS`, `ErrorS`, `Info`, `Infof`, `V(n)`, `KObj`, `KRef`,
`FromContext`, `Background`, `TODO`), or `logr.Logger` usage.
- Adds a `logger`/`ctx` parameter or a `…WithLogger` variant for contextual logging.
- Edits `hack/logcheck.conf`, `hack/tools` logcheck/logtools versions, or golangci-lint logging linters.
- Touches `k8s.io/component-base/logs` (incl. JSON logging) or `ktesting`.

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**.

@pohly pohly Jun 30, 2026 •
edited
Loading

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.

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).

🤷

@CatherineF-dev CatherineF-dev Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. 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.

  1. 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"
  1. 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.

@pohly pohly Jul 3, 2026

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.

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.

@CatherineF-dev CatherineF-dev Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@CatherineF-dev CatherineF-dev Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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.

Screenshot 2026-07-10 at 9 40 20 AM Screenshot 2026-07-10 at 9 56 19 AM


### A. Structured form
- **BLOCKER**: new unstructured `klog.Info/Infof/Errorf/Warningf` in a migrated package — use
`InfoS`/`ErrorS`. `ErrorS(err, msg, kv...)` takes the error **first**. Use it for log output that needs admin attention, otherwise prefer `InfoS`. `ErrorS` may be called with nil error. `InfoS` may be called with `"err": err` to log an error.
genuinely no error).
- Message must be a **constant string** (no `fmt.Sprintf`), capitalized, no trailing punctuation/newline.
- Keys are **lowerCamelCase**, stable, and reused (don't invent a new key for an existing concept).
- Object values use `klog.KObj(obj)` / `klog.KRef(ns, name)`, not `%s`/`%v` formatting.

### B. Contextual logging (KEP-3077)
- Obtain the logger from context: `logger := klog.FromContext(ctx)`; libraries receive a logger via
ctx rather than calling `klog.Background()`.
- New public functions that log should accept `ctx`/`logger`. A `ctx` is preferred if the function supports cancellation, otherwise a `logger`.
- `WithValues` for repeated key/value pairs; `WithName` to scope a component's logger.
- **QUESTION/BLOCKER** on `context.Background()`/`context.TODO()` in new code without a justifying
comment + tracking issue.

### C. logcheck / tooling
- If the PR migrates a package, it must **add that package to `hack/logcheck.conf`** so enforcement
sticks; conversely, don't enable a directory whose prerequisites aren't met.
- logcheck runs via golangci-lint; logtools/logcheck version bumps live in `hack/tools`. Confirm CI
(`pull-kubernetes-verify` / golangci-lint) covers the change.

### D. Verbosity
- Right `V()` level: `V(0)` important/always, `V(2)` useful steady-state, `V(4)` debug, `V(5)` trace.
Flag high-cost values logged at low verbosity, and secrets/PII logged at any level.

### E. Events
- Contextual event migration is **all-or-nothing** per area; grep for stragglers. Plugins should be
able to attach key/values via `EventRecorderLogger`.

### F. Tests
- Log-output assertions use `ktesting` with per-test isolation, not the global logger. Behavior tests
should still exercise the real code path.

## Step 4 — Output
```
## SIG-Instrumentation Logging Review — PR #<n>: <title>

**Scope:** <which logging/files this touches>
**Verdict:** /lgtm | /approve | changes requested | needs SIG discussion

### Blockers
- [file:line] <issue> — <why> → <fix>

### Nits
- [file:line] <suggestion>

### Questions for author
- <question>

### Required before merge
- make verify WHAT=verify-golangci-lint (logcheck runs here)
- ensure migrated packages are added to hack/logcheck.conf
```
Be concrete, cite lines, explain the *why* (JSON searchability, contextual-logging correctness,
cancellation propagation) — not just the rule.

## Reference
- `OWNERS_ALIASES` → `sig-instrumentation-approvers`.
- Structured logging: `kubernetes/community` → `contributors/devel/sig-instrumentation/migration-to-structured-logging.md`
(KEP-1602); contextual logging: `…/contextual-logging.md` (KEP-3077).
- logcheck lives in `sigs.k8s.io/logtools`; config at `hack/logcheck.conf`.
- klog API & `ktesting`: `k8s.io/klog/v2`.