Add option for configuring initial fields on the logger #244
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new function named Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
pkg/logging/logging.go (1)
46-50: LGTM! Consider minor enhancements for robustness.The
WithInitialFieldsfunction is well-implemented and aligns perfectly with the existing Option pattern. It provides the desired functionality of allowing users to set initial fields for the logger.To further improve robustness, consider the following suggestions:
Add input validation to handle nil or empty maps:
func WithInitialFields(fields map[string]interface{}) Option { return func(c *zap.Config) { if fields != nil && len(fields) > 0 { if c.InitialFields == nil { c.InitialFields = make(map[string]interface{}) } for k, v := range fields { c.InitialFields[k] = v } } } }This approach merges the new fields with existing ones (if any) instead of overwriting them, which might be more flexible in some scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- pkg/logging/logging.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
pkg/logging/logging.go (1)
46-50: Summary: PR objectives met with well-implemented changesThis pull request successfully introduces the ability to configure initial fields for the logger within the Baton SDK. The implementation:
- Adds a new
WithInitialFieldsoption that integrates seamlessly with the existing logging configuration system.- Maintains backward compatibility by not modifying existing functions.
- Enhances log context and traceability by allowing static values (like tenant_id, connector_id, and app_id) to be included in all log entries.
The changes align well with the PR objectives and follow good coding practices. Minor suggestions for improvement have been provided to enhance robustness.
To fully validate the implementation:
- Ensure that the verification script is run to check usage across the codebase.
- Consider adding unit tests for the new
WithInitialFieldsfunction.- Update documentation to reflect this new configuration option.
Overall, this is a valuable addition to the SDK that will improve logging capabilities for connectors.
| func WithInitialFields(fields map[string]interface{}) Option { | ||
| return func(c *zap.Config) { | ||
| c.InitialFields = fields | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
WithInitialFields is not used in the codebase.
- No instances of
WithInitialFieldsfound in anylogging.Initfunction calls.
🔗 Analysis chain
Integration looks good. Verify usage in the codebase.
The WithInitialFields function integrates seamlessly with the existing codebase. It follows the established Option pattern and can be used alongside other options in the Init function without any changes to the existing code.
To ensure proper usage across the codebase:
This script will help verify that the new WithInitialFields option is being used correctly throughout the project.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of WithInitialFields in the codebase
# Test 1: Check for imports of the logging package
echo "Checking for imports of the logging package:"
rg --type go 'import\s+\([^)]*"github\.com/ConductorOne/baton-sdk/pkg/logging"[^)]*\)' || rg --type go 'import\s+"github\.com/ConductorOne/baton-sdk/pkg/logging"'
# Test 2: Look for usage of WithInitialFields
echo "Checking for usage of WithInitialFields:"
rg --type go 'logging\.WithInitialFields\('
# Test 3: Examine Init function calls with options
echo "Examining Init function calls with options:"
rg --type go 'logging\.Init\(.*\)' -A 5
Length of output: 1106
| @@ -43,6 +43,12 @@ func WithOutputPaths(paths []string) Option { | |||
| } | |||
| } | |||
|
|
|||
| func WithInitialFields(fields map[string]interface{}) Option { | |||
There was a problem hiding this comment.
I'd add a docstring explaining what this does:
// WithInitialFields - ensure that the logger we create for the connector has static values included on logs.
func WithInitialFields(fields map[string]interface{}) Option {
...
}
calling something like:
wil be sure that the logger we create for the connector has static values included on logs.
Summary by CodeRabbit
New Features
Bug Fixes