feature: kgo_driver log split key an value #527
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds helper toKeyValuePair to convert variadic keyvals into []zapcore.Field with deterministic fallback naming; Log now uses that helper to build fields. Adds unit tests verifying behavior for nil, even string-key pairs, odd-length inputs, and mixed-type inputs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Logger
participant Helper as toKeyValuePair
participant Zap as zap.Logger
Caller->>Logger: Log(level, msg, keyvals...)
Logger->>Helper: toKeyValuePair(keyvals)
alt keyvals empty
Helper-->>Logger: nil
Logger->>Zap: log(level, msg)
else even-length && keys are strings
Helper-->>Logger: fields (zap.Any key/value)
Logger->>Zap: log(level, msg, fields)
else fallback (odd-length or non-string keys)
Helper-->>Logger: fields (val0, val1, ...)
Logger->>Zap: log(level, msg, fields)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
kafkajobs/logger.go (3)
37-37: Unify on zap.Field for clarity.Declare fields as []zap.Field (zap’s alias) to avoid mixing zapcore.Field/zap.Field.
- var zf []zapcore.Field + var zf []zap.Field
42-45: Minor loop readability nit.Iterate values directly; avoids indexing boilerplate.
- for i := range keyvals { - zf = append(zf, zap.Any("kgo_driver", keyvals[i])) - } + for _, v := range keyvals { + zf = append(zf, zap.Any("kgo_driver", v)) + }
62-79: Return []zap.Field from helper to keep types consistent.The function already builds []zap.Field; returning []zap.Field tightens the API and matches the logger call sites.
-func tryFetchKeyValPairs(keyvals []any) ([]zapcore.Field, bool) { +func tryFetchKeyValPairs(keyvals []any) ([]zap.Field, bool) {Optional: consider a more forgiving parse that:
- parses the longest valid prefix of key/value pairs, and
- returns any leftover values to be appended under "kgo_driver".
This prevents losing structured context when the input is “mostly” pairs but ends odd.kafkajobs/logger_test.go (4)
22-25: Fix typo in test name.Spelling nit: “keycals” → “keyvals”.
- name: "keycals is nil", + name: "keyvals is nil",
55-64: Isolate observer per subtest.Create a fresh observer/core inside each t.Run to avoid cross-test coupling and make failures easier to diagnose. Also assert length after TakeAll.
- core, o := observer.New(zapcore.DebugLevel) - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - newLogger(zap.New(core)). - Log(kgo.LogLevelDebug, c.name, c.keyvals...) - assert.Equal(t, 1, o.Len()) - logs := o.TakeAll() - assert.Equal(t, c.expected, logs[0].Context) - }) - } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + core, o := observer.New(zapcore.DebugLevel) + newLogger(zap.New(core)).Log(kgo.LogLevelDebug, c.name, c.keyvals...) + logs := o.TakeAll() + require.Len(t, logs, 1) + assert.Equal(t, c.expected, logs[0].Context) + }) + }
44-45: Clarify test name.“no strings” → “no string keys” better reflects the scenario.
- name: "no strings", + name: "no string keys",
67-111: Add a case for partial pairs (odd-length with valid prefix).To lock in behavior when input is “mostly pairs” but ends with a trailing value, add a test e.g.: []any{"k1","v1","k2"} expecting fields for k1/v1 and a single kgo_driver for "k2" if you adopt the partial-parse approach suggested in code review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
kafkajobs/logger.go(2 hunks)kafkajobs/logger_test.go(1 hunks)
🔇 Additional comments (1)
kafkajobs/logger.go (1)
39-41: Good call: structured key/value parsing path.Using a dedicated helper to parse key/value pairs makes logs parseable and avoids duplicate "kgo_driver" fields.
kafkajobs/logger.go
Outdated
| for i := range keyvals { | ||
| zf = append(zf, zap.Any("kgo_driver", keyvals[i])) | ||
| if zfTmp, ok := tryFetchKeyValPairs(keyvals); ok { | ||
| zf = zfTmp | ||
| } else { | ||
| zf = make([]zap.Field, 0, len(keyvals)) | ||
| for i := range keyvals { | ||
| zf = append(zf, zap.Any("kgo_driver", keyvals[i])) | ||
| } |
There was a problem hiding this comment.
I guess that you don't need to do anything here, but just remove kgo_driver part completely to have a clean logs:
for i := range keyvals {
zf = append(zf, zap.Any("kgo_driver", keyvals[i]))
}There was a problem hiding this comment.
Do you think we can be completely sure that keyvals always have pairs?
Like {"key1", val1, "key2": val2, ...}
There was a problem hiding this comment.
You may double-check that, but I think that you should always have an even number of fields. If not, you may always add some placeholder at the beginning/end of the sequence and apply the same algorithm for an even number of vals.
You may also look at the kgo/logger.go for the BasicLogger and implement a writer interface for our logger and pass it to the kgo BasicLogger.
There was a problem hiding this comment.
Yeah, it should be in the key: value format. So you can always expect an even number of keys (but with a double check to avoid panicking on out-of-bound access).
|
@rustatian PR in draft because I going to build RR with velox and my PR to test it. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes duplicate "kgo_driver" keys in logging output from the Kafka Franz-go library. The implementation adds key-value pair parsing to split structured log entries instead of using a single "kgo_driver" key for all values.
- Introduces
tryFetchKeyValPairsfunction to parse alternating key-value pairs from log arguments - Updates the
Logmethod to attempt structured parsing first, falling back to the original behavior - Adds comprehensive unit tests for both the logger and the new parsing functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| kafkajobs/logger_test.go | New test file with comprehensive test cases for logger functionality and key-value parsing |
| kafkajobs/logger.go | Updates Log method to parse key-value pairs and adds tryFetchKeyValPairs helper function |
| go.work.sum | Updates to dependency checksums (not directly related to the core change) |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
rustatian
changed the title
Co-authored-by: Copilot <[email protected]>
|
OK. I've build RR with velox and my branch LGTM 
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
kafkajobs/logger.go (1)
84-89: Clarify the fallback comment to reflect reality.This branch intentionally handles odd-length or non-string-key inputs (and the new tests cover it), so labeling it “should never happen” is misleading. Please update the comment to describe the fallback behavior.
- // This should never happen. + // Fallback when keyvals are not well-formed key/value pairs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kafkajobs/logger.go(3 hunks)kafkajobs/logger_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kafkajobs/logger_test.go
🔇 Additional comments (1)
kafkajobs/logger.go (1)
61-81: Good defensive handling of key/value slices.Nice to see the even-length gate and string-key validation before constructing the zap fields; that keeps us aligned with franz-go’s expectations while preserving the original key names.
| inLen := len(keyvals) | ||
| if inLen == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Do not call checks for the function to be executed inside the callee function. It is better to check that condition before entering the function.
| keys := make([]string, 0, inLen/2) | ||
| for i := 0; i < inLen; i += 2 { | ||
| if key, ok := keyvals[i].(string); ok { | ||
| keys = append(keys, key) | ||
| } else { | ||
| isKVP = false | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // positive scenario | ||
| if isKVP { | ||
| result := make([]zap.Field, inLen/2) | ||
| for i, key := range keys { | ||
| result[i] = zap.Any(key, keyvals[i*2+1]) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
This is the hot place, don't iterate twice here (keys check + keys add to the result).
Remove isKVP, because if by some reason the number is odd, as I suggested, just add some placeholder at the end of the sequence with some predefined text.
Preallocate result slice and while iterating - add key-value pairs to the result.
Do not cast values to string type, it does not matter since you're using zap.Any.
Co-authored-by: Copilot <[email protected]>
2 participants
Reason for This PR
Currently, the logs contain multiple duplicate "kgo_driver" keys. It's difficult to read or parse such logs.
Example of "hard to understand what's going on" log:
{ "level":"debug", "logger": "kafka.kgo", "msg":"read Fetch v15", "kgo_driver":"broker", "kgo_driver":"1", "kgo_driver":"bytes_read", "kgo_driver":21, "kgo_driver":"read_wait", "kgo_driver":0.000033985, "kgo_driver":"time_to_read", "kgo_driver":0.000234346, "kgo_driver":"err", "kgo_driver":null }Description of Changes
I simply add key-value splitter to Log function and some tests.
Now we will have pretty logs like:
{ "level": "debug", "logger": "kafka.kgo", "msg": "read Fetch v15", "broker": "1", "bytes_read": 21, "read_wait": 0.000033985, "time_to_read": 0.000234346, "err": null }License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Bug Fixes
Refactor
Tests