feat(pii): optional IP tracking #84
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR introduces optional client IP tracking functionality to support PII/GDPR compliance scenarios, allowing users to disable IP address logging when needed. The changes also include refactoring of the configuration initialization and performance optimizations through pre-allocated slice capacities.
- Added a new
WithClientIPconfiguration field to control whether client IP addresses are logged (defaults totrue) - Refactored configuration initialization by introducing a
DefaultConfig()function for better code reusability - Optimized memory allocations by pre-allocating slice capacities for attributes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| middleware.go | Adds WithClientIP config field, creates DefaultConfig() function, implements conditional IP logging, and optimizes slice allocations |
| README.md | Updates documentation to include the new WithClientIP configuration field |
| .github/workflows/test.yml | Disables fail-fast for the test matrix to allow all Go version tests to complete |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| baseAttributes := []slog.Attr{} | ||
| baseAttributes := make([]slog.Attr, 0, 3) | ||
| requestAttributes := make([]slog.Attr, 0, 14) |
There was a problem hiding this comment.
The pre-allocated capacity for requestAttributes is set to 14, but the initial append only adds 8 attributes. The actual number of attributes varies depending on configuration flags (WithClientIP, WithRequestBody, WithRequestHeader, WithUserAgent), and the X-Forwarded-For header presence. Consider adjusting the capacity to better match the typical case, or use a more conservative estimate that accounts for the most common configuration.
| requestAttributes := make([]slog.Attr, 0, 14) | |
| requestAttributes := make([]slog.Attr, 0, 8) |
| slog.String("ip", ip), | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on this line (tab character after the closing parenthesis). This should be removed for consistency with the codebase formatting.
| WithResponseHeader bool | ||
| WithSpanID bool | ||
| WithTraceID bool | ||
| WithClientIP bool |
There was a problem hiding this comment.
The README has been updated to include the new WithClientIP field in the Config struct documentation, but there's no explanation of what this field does, what its default value is, or when users might want to set it to false (e.g., for PII/GDPR compliance). Consider adding a usage example or documentation section that explains this privacy-focused feature, especially given the PR title mentions "optional IP tracking".
|
|
||
| Filters: []Filter{}, | ||
| }) | ||
| return NewWithConfig(logger, Config{ }) |
There was a problem hiding this comment.
The New() function is passing an empty Config{} struct instead of using DefaultConfig(). This means all configuration fields will be set to their zero values, which differs from the intended default behavior. For example, WithRequestID should default to true and WithClientIP should default to true according to DefaultConfig(), but they will be false with an empty struct. This will break existing users who expect the default configuration when calling New().
| return NewWithConfig(logger, Config{ }) | |
| return NewWithConfig(logger, DefaultConfig()) |
No description provided.