feat: [#282] Print the values in ctx when calling facades.Log().WithContext(ctx).Info() #759
Conversation
…ontext(ctx).Info()
WalkthroughThis pull request introduces significant enhancements to the logging system in the Goravel framework. The changes focus on expanding the Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #759 +/- ##
==========================================
+ Coverage 69.82% 69.88% +0.06%
==========================================
Files 210 212 +2
Lines 17853 17890 +37
==========================================
+ Hits 12465 12503 +38
- Misses 4707 4708 +1
+ Partials 681 679 -2 ☔ View full report in Codecov by Sentry. |
| if value, exists := root[key]; exists && value != nil { | ||
| v, err := general.json.Marshal(value) |
There was a problem hiding this comment.
Not all values can be marshaled to json, we can print them simply.
log/logrus_writer.go
Outdated
| @@ -333,7 +307,6 @@ func registerHook(config config.Config, json foundation.Json, instance *logrus.L | |||
|
|
|||
| if config.GetBool(channelPath + ".print") { | |||
| instance.SetOutput(os.Stdout) | |||
| instance.SetFormatter(formatter.NewGeneral(config, json)) | |||
There was a problem hiding this comment.
Had set this in hook.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (8)
log/utils_test.go (1)
29-34: Ensure test covers all expected context valuesThe test currently checks for specific keys but may miss cases where additional or unexpected keys are present.
Suggestion: Use
assert.EqualValuesfor a more thorough comparisonConsider using
assert.EqualValuesto compare the expected and actual maps, ensuring that both contain exactly the same key-value pairs.assert.EqualValues(t, map[any]any{ "a": "b", "c": map[string]any{"d": "e"}, "d": T{A: "a"}, }, values)log/entry.go (2)
11-25: Consistent ordering of struct fields for better readabilityThe fields in the
Entrystruct appear to be reordered without an obvious pattern. Consistently grouping related fields enhances readability and maintainability.Suggestion: Group fields logically
Consider grouping fields like
code,domain,hint, andmessagetogether, as they all pertain to the log entry's identity, and group context-related fields separately.
40-47: Add comments to exported methodsDomain()andHint()Exported methods should have comments to comply with Go's documentation standards. This aids in generating proper documentation and helps other developers understand the purpose of these methods.
Suggestion: Add method comments
// Domain returns the domain associated with the log entry. func (r *Entry) Domain() string { return r.domain } // Hint returns the hint associated with the log entry. func (r *Entry) Hint() string { return r.hint }log/formatter/general_test.go (1)
138-141: Ensure consistent test coverageThe test assertions for formatted data match those in the Format test. This consistency is good, but consider adding edge cases for different value types (integers, booleans, etc.).
log/logrus_writer_test.go (4)
53-62: Enhance context testing coverageThe current test case only verifies a simple key-value pair. Consider adding test cases for:
- Multiple context values
- Nested context values
- Nil context values
func TestLogrus(t *testing.T) { // ... existing code ... + { + name: "WithContext_Multiple", + setup: func() { + mockDriverConfig(mockConfig) + log, err = NewApplication(mockConfig, j) + ctx := context.Background() + ctx = context.WithValue(ctx, testContextKey("key1"), "value1") + ctx = context.WithValue(ctx, testContextKey("key2"), "value2") + log.WithContext(ctx).Info("Goravel") + }, + assert: func() { + assert.True(t, file.Contain(singleLog, "test.info: Goravel\ncontext: map[key1:value1 key2:value2]")) + assert.True(t, file.Contain(dailyLog, "test.info: Goravel\ncontext: map[key1:value1 key2:value2]")) + }, + }, + { + name: "WithContext_Nested", + setup: func() { + mockDriverConfig(mockConfig) + log, err = NewApplication(mockConfig, j) + ctx := context.Background() + ctx = context.WithValue(ctx, testContextKey("key"), map[string]interface{}{ + "nested": "value", + }) + log.WithContext(ctx).Info("Goravel") + }, + assert: func() { + assert.True(t, file.Contain(singleLog, "test.info: Goravel\ncontext: map[key:map[nested:value]]")) + assert.True(t, file.Contain(dailyLog, "test.info: Goravel\ncontext: map[key:map[nested:value]]")) + }, + },
221-222: Add combined metadata test casesThe current test cases verify metadata fields individually. Consider adding test cases that combine multiple metadata fields to ensure they work together correctly.
func TestLogrus(t *testing.T) { // ... existing code ... + { + name: "Combined_Metadata", + setup: func() { + mockDriverConfig(mockConfig) + log, err = NewApplication(mockConfig, j) + log.Code("error_code"). + Hint("error_hint"). + In("error_domain"). + Info("Goravel") + }, + assert: func() { + assert.True(t, file.Contain(singleLog, "test.info: Goravel\ncode: error_code\nhint: error_hint\ndomain: error_domain")) + assert.True(t, file.Contain(dailyLog, "test.info: Goravel\ncode: error_code\nhint: error_hint\ndomain: error_domain")) + }, + },Also applies to: 234-235, 247-248
275-280: Enhance HTTP request/response test coverageThe current test cases only cover basic HTTP scenarios. Consider adding test cases for:
- Different HTTP methods (POST, PUT, DELETE)
- Query parameters
- Different response types (JSON, XML)
- Error responses
Also, the current request test assertions could be more precise by using specific string matching instead of partial matches:
- expectedParts := []string{ - `test.info: Goravel`, - `request: `, - `method:GET`, - `uri:http://localhost:3000/`, - `Sec-Fetch-User:[?1]`, - `Host:[localhost:3000]`, - `body:map[key1:value1 key2:value2]`, - } + expected := "test.info: Goravel\nrequest: map[method:GET uri:http://localhost:3000/ headers:map[Host:[localhost:3000] Sec-Fetch-User:[?1]] body:map[key1:value1 key2:value2]]" + assert.True(t, file.Contain(singleLog, expected)) + assert.True(t, file.Contain(dailyLog, expected))Also applies to: 300-304
260-261: Add edge cases for user/owner/tags testsThe current test cases only cover happy paths. Consider adding test cases for:
- Multiple tags
- Complex user metadata (nested objects)
- Invalid owner email format
func TestLogrus(t *testing.T) { // ... existing code ... + { + name: "Multiple_Tags", + setup: func() { + mockDriverConfig(mockConfig) + log, err = NewApplication(mockConfig, j) + log.Tags("tag1", "tag2", "tag3").Info("Goravel") + }, + assert: func() { + assert.True(t, file.Contain(singleLog, "test.info: Goravel\ntags: [tag1 tag2 tag3]")) + assert.True(t, file.Contain(dailyLog, "test.info: Goravel\ntags: [tag1 tag2 tag3]")) + }, + }, + { + name: "Complex_User", + setup: func() { + mockDriverConfig(mockConfig) + log, err = NewApplication(mockConfig, j) + log.User(map[string]any{ + "name": "kkumar-gcc", + "roles": []string{"admin", "user"}, + "metadata": map[string]any{ + "last_login": "2024-01-01", + }, + }).Info("Goravel") + }, + assert: func() { + assert.True(t, file.Contain(singleLog, "test.info: Goravel\nuser: map[name:kkumar-gcc roles:[admin user] metadata:map[last_login:2024-01-01]]")) + assert.True(t, file.Contain(dailyLog, "test.info: Goravel\nuser: map[name:kkumar-gcc roles:[admin user] metadata:map[last_login:2024-01-01]]")) + }, + },Also applies to: 322-323, 335-336
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
mocks/log/Entry.gois excluded by!mocks/**
📒 Files selected for processing (11)
contracts/log/log.go(1 hunks)log/entry.go(2 hunks)log/formatter/general.go(1 hunks)log/formatter/general_test.go(2 hunks)log/hook.go(1 hunks)log/hook_test.go(1 hunks)log/logrus_writer.go(4 hunks)log/logrus_writer_test.go(10 hunks)log/utils.go(1 hunks)log/utils_test.go(1 hunks)support/file/file.go(1 hunks)
🔇 Additional comments (8)
support/file/file.go (1)
24-24: LGTM: Improved code readability
The added blank line improves readability by separating error handling from the return statement.
log/formatter/general.go (2)
85-85: LGTM: Added context value support
The addition of "with" key to the list of processed keys aligns with the PR objective to print context values.
87-87: LGTM: Improved value formatting
Replacing JSON marshaling with direct string formatting is a good optimization that handles non-JSON-marshalable values better.
log/hook_test.go (1)
15-149: LGTM: Comprehensive test coverage
The test implementation is thorough and well-structured:
- Covers all scenarios: full data, empty data, and error cases
- Properly verifies context value handling
- Uses table-driven tests for better maintainability
- Includes proper mock expectations and assertions
contracts/log/log.go (1)
107-110: LGTM: Interface additions are well-structured
The new Domain() and Hint() methods are consistent with the existing interface pattern and provide clear access to additional contextual information.
log/formatter/general_test.go (1)
74-77: Verify test assertions match actual formatting
The test assertions have been updated to expect unquoted values. Please verify that this matches the actual formatting implementation in the formatter.
log/logrus_writer.go (2)
28-40: LGTM: Well-structured field additions
The new fields (code, domain, hint, etc.) are properly organized and initialized. The type choices are appropriate for their purposes.
270-272: LGTM: Proper handling of additional context
The with field is correctly included in the payload only when it contains values, following the pattern used for other fields.
| `response: {`, | ||
| `"status":200`, | ||
| `"header":{"Content-Type":["text/plain; charset=utf-8"]}`, | ||
| `"body":{}`, |
There was a problem hiding this comment.
@kkumar-gcc The body is missing when usnig json.Marshal, and map[any]any can't be marshaled, golang throws an error.
There was a problem hiding this comment.
@hwbrzzl I believe the reason the body is missing is that we should use res.Origin().Body().String() because res.Origin().Body() is a buffer. However, map[any]any cannot be parsed.
There was a problem hiding this comment.
Yes, so we don't need to use json.Marshal, printing them should be fine.
There was a problem hiding this comment.
Well, it will include map and other type information, which I’m not sure is a good idea to include in the logs.
There was a problem hiding this comment.
Yes, it's fine, I prefer to know the specific type.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: c447238 | Previous: bc4546a | Ratio |
|---|---|---|---|
Benchmark_Fatal |
2e-7 ns/op 0 B/op 0 allocs/op |
1e-7 ns/op 0 B/op 0 allocs/op |
2 |
Benchmark_Fatal - ns/op |
2e-7 ns/op |
1e-7 ns/op |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
log/logrus_writer.go (1)
227-233:⚠️ Potential issueCritical: Add validation for context values
The current implementation extracts context values without any validation or sanitization, which could lead to:
- Nil pointer dereferences
- Non-serializable types causing logging failures
- Potential memory leaks from large contexts
- Security risks from sensitive data exposure
Apply this diff to add validation:
if ctx := r.instance.Context; ctx != nil { values := make(map[any]any) getContextValues(ctx, values) if len(values) > 0 { + // Sanitize and validate context values + sanitizedValues := make(map[string]any) + for k, v := range values { + if v == nil { + continue + } + // Convert keys to strings for consistency + key := fmt.Sprint(k) + // Skip sensitive keys + if isSensitiveKey(key) { + continue + } + // Validate value type and size + if sanitized, ok := sanitizeValue(v); ok { + sanitizedValues[key] = sanitized + } + } - payload["context"] = values + if len(sanitizedValues) > 0 { + payload["context"] = sanitizedValues + } } }Add these helper functions:
func isSensitiveKey(key string) bool { sensitive := []string{"password", "token", "secret", "key", "auth"} for _, s := range sensitive { if strings.Contains(strings.ToLower(key), s) { return true } } return false } func sanitizeValue(v any) (any, bool) { switch val := v.(type) { case string: if len(val) > 1000 { return val[:1000] + "...", true } return val, true case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64, bool: return val, true default: // Convert other types to string representation return fmt.Sprint(v), true } }
🧹 Nitpick comments (2)
log/application.go (1)
41-44: Consider adding error handling for invalid context typesWhile the HTTP context handling is a good addition, consider adding validation and error handling for cases where the context might be nil or invalid.
func (r *Application) WithContext(ctx context.Context) log.Writer { + if ctx == nil { + return NewWriter(r.instance.WithContext(context.Background())) + } + if httpCtx, ok := ctx.(http.Context); ok { + if httpCtx.Context() == nil { + return NewWriter(r.instance.WithContext(context.Background())) + } return NewWriter(r.instance.WithContext(httpCtx.Context())) } return NewWriter(r.instance.WithContext(ctx)) }log/logrus_writer.go (1)
29-41: Consider adding field validation in NewWriterThe Writer struct has several fields that could benefit from validation during initialization.
func NewWriter(instance *logrus.Entry) log.Writer { + if instance == nil { + instance = logrus.NewEntry(logrus.New()) + } return &Writer{ code: "", domain: "", hint: "", instance: instance, message: "", owner: nil, request: nil, response: nil, stackEnabled: false, stacktrace: nil, tags: []string{}, user: nil, with: map[string]any{}, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
log/application.go(2 hunks)log/formatter/general.go(3 hunks)log/formatter/general_test.go(4 hunks)log/logrus_writer.go(4 hunks)log/logrus_writer_test.go(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- log/logrus_writer_test.go
- log/formatter/general_test.go
- log/formatter/general.go
📑 Description
Closes goravel/goravel#282
Summary by CodeRabbit
Summary by CodeRabbit
New Features
code,domain, andhint.Hooktype for improved integration with the logging framework.getContextValuesfor extracting key-value pairs from context structures.WithContextmethod to handle HTTP-specific contexts.Bug Fixes
Tests
Hookfunctionality and context value extraction.Chores
✅ Checks