feat(pii): optional IP tracking#72
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #72 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 4 4
Lines 368 365 -3
=====================================
+ Misses 368 365 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces optional IP address tracking to the middleware logger, allowing users to control whether client IP addresses (potentially PII) are logged. The implementation adds a new WithClientIP configuration field that defaults to true to maintain backward compatibility with existing behavior.
Key changes:
- Added
WithClientIPboolean field to theConfigstruct for controlling IP logging - Refactored configuration management by extracting a
DefaultConfig()function - Modified IP logging to be conditional based on the
WithClientIPconfiguration flag
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| middleware.go | Added WithClientIP field to Config struct, extracted DefaultConfig() function, made IP logging conditional, and optimized slice pre-allocation |
| README.md | Updated Config struct documentation to include the new WithClientIP field |
| .github/workflows/test.yml | Added fail-fast: false to test matrix configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // DefaultConfig returns the default configuration for the request logger. | ||
| func DefaultConfig() Config { | ||
| return Config{ | ||
| DefaultLevel: slog.LevelInfo, |
There was a problem hiding this comment.
Inconsistent indentation. The DefaultLevel field has extra tabs/spaces compared to the other fields in the struct initialization.
| DefaultLevel: slog.LevelInfo, | |
| DefaultLevel: slog.LevelInfo, |
| test: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
The addition of fail-fast: false to the workflow configuration appears unrelated to the PR's purpose of adding optional IP tracking. This change affects CI/CD behavior by allowing all matrix jobs to complete even if one fails. If this is intentional, it should be in a separate PR or explained in the PR description.
| WithResponseHeader bool | ||
| WithSpanID bool | ||
| WithTraceID bool | ||
| WithClientIP bool |
There was a problem hiding this comment.
The new WithClientIP configuration field is added to the Config struct but lacks documentation explaining its purpose and usage. Given that this is a privacy-related feature (PII tracking), it would be helpful to add a comment or example showing how to disable IP logging for privacy compliance. Consider adding a usage example such as:
config := sloggin.Config{
WithClientIP: false, // Disable IP logging for GDPR/privacy compliance
}|
|
||
| Filters: []Filter{}, | ||
| }) | ||
| return NewWithConfig(logger, Config{}) |
There was a problem hiding this comment.
This change breaks backward compatibility. The New() function previously returned a Config with sensible defaults (DefaultLevel: slog.LevelInfo, ClientErrorLevel: slog.LevelWarn, etc.), but now returns an empty Config with zero values. This means:
config.DefaultLevelwill be 0 (slog.LevelDebug) instead of slog.LevelInfoconfig.ClientErrorLevelandconfig.ServerErrorLevelwill also be 0 instead of their intended valuesconfig.WithRequestIDwill be false instead of trueconfig.WithClientIPwill be false instead of the new default of true
This breaks existing code using New() and changes the logging behavior in ways users won't expect. The function should return DefaultConfig() instead of an empty Config.
| return NewWithConfig(logger, Config{}) | |
| return NewWithConfig(logger, DefaultConfig()) |
No description provided.