Skip to content

fix(restyutil): logError → slog.Error per @mliu33; delete unused pkg/model/Employee#163

Merged
Joey0538 merged 1 commit intomainfrom
claude/restyutil-logging-tweaks
May 8, 2026
Merged

fix(restyutil): logError → slog.Error per @mliu33; delete unused pkg/model/Employee#163
Joey0538 merged 1 commit intomainfrom
claude/restyutil-logging-tweaks

Conversation

@GITMateuszCharczuk
Copy link
Copy Markdown
Collaborator

@GITMateuszCharczuk GITMateuszCharczuk commented May 8, 2026

Summary

Two changes:

  1. logErrorslog.Error (per @mliu33's review on PR feat(pkg): natsrouter improvements + mongoutil bulk API + new minioutil #157). The codebase convention for "we attempted an operation and it returned an error" is consistently slog.Error (mongoutil.Disconnect, natsutil.Reply, natsutil.Ack, natsrouter handler-error). Transport-level failures from resty's OnError (connect refused, DNS, ctx-deadline) fit that pattern. Was hiding real outages behind --debug.

  2. Delete pkg/model/employee.go. Zero callers in the repo; bring it back when a real consumer shows up.

On @mliu33's Q2 ("include resp.StatusCode when logging error response")

Already satisfied by the existing code — logResponse calls logFields(req, resp.StatusCode(), ...) which appends a status field on every record. No code change needed there; logResponse stays at slog.Debug.

Test plan

  • TestLog_FiresOnTransportError strengthened with a "level":"ERROR" assertion
  • make test — 10 restyutil tests pass
  • go build ./... and go build -tags=integration ./... — exit 0
  • make lint — 0 issues
  • grep -rn "model\.Employee" --include="*.go" — zero hits before deletion

Diff

pkg/restyutil/restyutil.go +1 / -1, pkg/restyutil/restyutil_test.go +1 / -1, pkg/model/employee.go deleted (-91 lines).

https://claude.ai/code/session_01Ngkd45NdAxeqX4FU4t6w9X

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The PR updates HTTP client logging in the Resty utility to use status-code-driven severity: 2xx/3xx responses log at debug level, 4xx at warn, and 5xx at error; transport errors log at error level. Test coverage verifies the dynamic severity selection and status code field presence.

Changes

Status-Driven HTTP Logging

Layer / File(s) Summary
Dependencies
pkg/restyutil/restyutil.go
Import context package to support context-aware slog calls.
Logging Logic
pkg/restyutil/restyutil.go
logResponse selects severity dynamically (2xx/3xx → debug, 4xx → warn, 5xx → error); logError elevated to error level.
Test Coverage
pkg/restyutil/restyutil_test.go
Transport error test asserts ERROR-level logging; new TestLog_StatusDrivenLogLevel verifies severity by status code and field presence; itoa helper supports assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit logs with grace so fine,
Status codes now match their line—
Debug for greens, and warns for fours,
Errors red for five and more!
Tests confirm each hop is right,
The logging hops through day and night. 🌙

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ⚠️ Warning Title mentions 'fix(restyutil): logError → slog.Error' and 'delete unused pkg/model/Employee', but the summary shows only restyutil logging changes with no mention of deleting Employee. Update the title to accurately reflect that this PR only addresses restyutil logging level changes, removing the misleading reference to pkg/model/Employee deletion.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/restyutil-logging-tweaks

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@GITMateuszCharczuk GITMateuszCharczuk force-pushed the claude/restyutil-logging-tweaks branch from 8f3ef33 to ab47802 Compare May 8, 2026 02:50
…l/Employee

Two changes:

1. logError uses slog.Error instead of slog.Debug, matching the
   codebase convention (mongoutil.Disconnect, natsutil.Reply/Ack,
   natsrouter handler-error all use Error). @mliu33 PR #157 review.

2. Delete pkg/model/employee.go entirely. The struct had zero
   callers in the repo. No replacement; bring back when a real
   user shows up.

logResponse stays at slog.Debug -- status code was already in the
log fields (logFields includes resp.StatusCode), no level change
needed beyond what @mliu33 actually requested.
@GITMateuszCharczuk GITMateuszCharczuk force-pushed the claude/restyutil-logging-tweaks branch from ab47802 to f94553c Compare May 8, 2026 02:50
@GITMateuszCharczuk GITMateuszCharczuk changed the title fix(restyutil): align log levels with codebase convention (per @mliu33) fix(restyutil): logError → slog.Error per @mliu33; delete unused pkg/model/Employee May 8, 2026
@Joey0538 Joey0538 merged commit 2940eec into main May 8, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants