Conversation
📝 WalkthroughWalkthroughThe PR adds map field support to the RPC code generator (Key/Value metadata, read/write, offsets, and generation paths) and a test for map generation. It introduces a map<string,string> attributes field to LogEntry with HasAttributes, Attributes, and SetAttributes methods and updates generated/internal data structures to store attributes. The CLI log command now formats and displays attributes, and the logs server collects non-"source" attributes and attaches them to serialized log entries. New testdata and generated examples for map types were added. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/commands/logs.go`:
- Around line 254-276: The function formatAttributes currently only quotes
values containing space, tab or double-quote; update the check in
formatAttributes to also detect newline and carriage-return characters so values
containing '\n' or '\r' are quoted (e.g., extend the strings.ContainsAny check
to include '\n' and '\r' or use an equivalent test) so multi-line attribute
values are emitted as quoted strings and do not split log entries across
terminal lines.
In `@pkg/rpc/generator.go`:
- Around line 282-284: The schema allows a `type: map` with missing key/value
which later breaks codegen in mapType; add a validation check during
schema-validate to reject malformed map declarations by ensuring any DescField
(or parameter) whose Type is Map has non-nil Key and Value (and that their types
are valid), returning a clear validation error message referencing the
field/parameter name; update the validator that walks field/parameter
descriptors (the same area that validates lists/records) so invalid maps fail
fast instead of reaching Generator.mapType.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7e41b19-d84e-4906-8789-4828989e896c
📒 Files selected for processing (8)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlcli/commands/logs.gopkg/rpc/generator.gopkg/rpc/generator_test.gopkg/rpc/testdata/map.gopkg/rpc/testdata/map.ymlservers/logs/logs.go
The RPC schema system previously had no way to express map fields,
forcing workarounds like list-of-NamedValue or encoding key=value
into strings. This adds a `type: map` with `key` and `value` fields
that generates idiomatic Go `map[K]V` with proper CBOR/JSON tags.
YAML syntax:
- name: attributes
type: map
key: string
value: string
index: 4
miren logs system was dropping all structured slog attributes (module, level, err, etc.) because toLogEntry() only extracted "source" and the LogEntry RPC type had no field for arbitrary metadata. Now attributes flow through the full pipeline and display as sorted key=value pairs. Fixes MIR-776
phinze
force-pushed
the
phinze/mir-776-miren-logs-system-drops-structured-log-attributes
branch
from
f9031a4 to
625f065
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/rpc/generator.go (1)
2189-2196:⚠️ Potential issue | 🟠 MajorValidate map method signatures too.
Line 2192 adds the new
mapvalidation forDescField, butDescParamaternow exposes the samekey/valuesurface at Lines 2332-2337 and still has no validation path. Atype: mapparameter/result with a missing side will still make it tomapType()and generate broken Go instead of failing schema validation.Also applies to: 2332-2337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rpc/generator.go` around lines 2189 - 2196, The schema validator currently checks map key/value only for DescField but not for DescParameter, so a parameter/result with type "map" can reach mapType() with empty Key/Value and generate broken Go; add the same validation logic used for fields to the parameter validation path (check DescParameter.Type == "map" and return a formatted error if Key=="" or Value==""), apply it to both parameters and results processing functions where DescParameter is validated so mapType() never receives an incomplete map description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/rpc/generator.go`:
- Around line 2189-2196: The schema validator currently checks map key/value
only for DescField but not for DescParameter, so a parameter/result with type
"map" can reach mapType() with empty Key/Value and generate broken Go; add the
same validation logic used for fields to the parameter validation path (check
DescParameter.Type == "map" and return a formatted error if Key=="" or
Value==""), apply it to both parameters and results processing functions where
DescParameter is validated so mapType() never receives an incomplete map
description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68936782-153a-481d-a32f-96c55d41142b
📒 Files selected for processing (8)
api/app/app_v1alpha/rpc.gen.goapi/app/rpc.ymlcli/commands/logs.gopkg/rpc/generator.gopkg/rpc/generator_test.gopkg/rpc/testdata/map.gopkg/rpc/testdata/map.ymlservers/logs/logs.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/rpc/generator_test.go
- servers/logs/logs.go
- pkg/rpc/testdata/map.yml
- api/app/rpc.yml
- api/app/app_v1alpha/rpc.gen.go
| func (g *Generator) mapType(field *DescField) *j.Statement { | ||
| return j.Map(g.properType(field.Key)).Add(g.properType(field.Value)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go, what types are valid as map keys, and which map key types does encoding/json support for Marshal and Unmarshal?
💡 Result:
Valid Go map key types (language rule)
A Go map key type must be a type for which the comparison operators == and != are fully defined—so the key type must not be a function, map, or slice type. If the key type is an interface, the dynamic (concrete) values stored in the interface must also be comparable, or comparisons may panic at runtime. [1]
Practically, this means map keys can be (among others): booleans, numbers, strings, pointers, channels, interfaces (with the caveat above), and arrays/structs as long as all their elements/fields are comparable. [1]
Map key types supported by encoding/json
json.Marshal supports map keys whose type is: any string type, any integer type, or a type that implements encoding.TextMarshaler. Integer keys are converted to strings; TextMarshaler keys use MarshalText() and become JSON strings. [2]
json.Unmarshal supports map keys whose type is: any string type, any integer type, or a type that implements encoding.TextUnmarshaler (decoded from the JSON object key string). [2]
Note: JSON object keys are strings, so non-string Go map keys are handled via these conversions. [2]
Sources: Go spec (map types/comparability) [1]; encoding/json docs for map key handling in Marshal/Unmarshal [2].
[1] (go.dev)
[2] (pkg.go.dev)
Citations:
🏁 Script executed:
# First, let's locate and examine the mapType function and its context
cd /tmp/repo 2>/dev/null || cd . && git ls-files "**/*.go" | head -20Repository: mirendev/runtime
Length of output: 583
🏁 Script executed:
# Look at the generator.go file around the mapType function
cat -n pkg/rpc/generator.go | sed -n '270,300p'Repository: mirendev/runtime
Length of output: 949
🏁 Script executed:
# Search for where mapType is called from and how keys are handled
rg -B 3 -A 3 "mapType" pkg/rpc/Repository: mirendev/runtime
Length of output: 3342
🏁 Script executed:
# Search for any schema validation related to map keys
rg -i "map.*key|key.*type|validate.*key" pkg/rpc/ --type goRepository: mirendev/runtime
Length of output: 831
🏁 Script executed:
# Find DescField definition
rg -B 5 -A 10 "type DescField struct" pkg/rpc/Repository: mirendev/runtime
Length of output: 715
🏁 Script executed:
# Look for schema validation or parsing
rg -i "validate|parse.*schema" pkg/rpc/ --type go | head -30Repository: mirendev/runtime
Length of output: 366
🏁 Script executed:
# Check test data to see what map key types are being used
fd "testdata" pkg/rpc/ -type dRepository: mirendev/runtime
Length of output: 230
🏁 Script executed:
# Look at map.go test data
cat pkg/rpc/testdata/map.goRepository: mirendev/runtime
Length of output: 1492
🏁 Script executed:
# Find and display the Validate method
rg -B 2 -A 30 "func \(t \*DescType\) Validate\(\)" pkg/rpc/generator.goRepository: mirendev/runtime
Length of output: 904
🏁 Script executed:
# Search for schema files to see what types are actually used
find pkg/rpc/testdata -name "*.yaml" -o -name "*.yml" 2>/dev/nullRepository: mirendev/runtime
Length of output: 249
🏁 Script executed:
# Check if there are any schema files with map fields
rg -l "type:.*map" --type yamlRepository: mirendev/runtime
Length of output: 100
🏁 Script executed:
# Look for any RPC schema examples
find . -name "*.yaml" -o -name "*.yml" | head -20Repository: mirendev/runtime
Length of output: 536
🏁 Script executed:
# Check the map.yml test schema
cat pkg/rpc/testdata/map.ymlRepository: mirendev/runtime
Length of output: 277
🏁 Script executed:
# Check a real API schema that uses maps
cat api/app/rpc.yml | head -100Repository: mirendev/runtime
Length of output: 2085
🏁 Script executed:
# Search for "map" in any schema files to see what key types are used
rg "type:.*map" -A 3 -B 1 --type yamlRepository: mirendev/runtime
Length of output: 457
🏁 Script executed:
# Search for all map declarations across all schema files
rg -A 5 "type:.*map" api/ --type yamlRepository: mirendev/runtime
Length of output: 252
🏁 Script executed:
# Check the properType function to see if it does any validation
rg -B 5 -A 15 "func \(g \*Generator\) properType" pkg/rpc/generator.goRepository: mirendev/runtime
Length of output: 428
🏁 Script executed:
# Look for any comments or documentation about supported map key types
rg -i "map.*key|key.*type.*valid|support.*key" pkg/rpc/ --type go -B 2 -A 2Repository: mirendev/runtime
Length of output: 1946
Add validation to reject unsupported map key types during schema parsing.
mapType() currently accepts any RPC schema key type without validation. Declarations like key: bytes or key: list would produce invalid Go map types (slices are not comparable and cannot be map keys), and according to the encoding/json package, only string, integer, and TextMarshaler implementations are supported as map keys. This validation should be added to DescType.Validate() instead of silently accepting invalid keys during code generation.
phinze
deleted the
phinze/mir-776-miren-logs-system-drops-structured-log-attributes
branch
2 participants
miren logs systemwas showing bare messages without any of the structured context that slog attaches — nomodule,level,err, nothing. If you wanted to see what was actually going on, you had to SSH in and read journalctl. The data was there in VictoriaLogs the whole time; it just got dropped on the way out to the CLI.The root cause was two layers deep. The
LogEntryRPC type had no way to carry arbitrary key-value metadata, andtoLogEntry()was only plucking out thesourceattribute and throwing the rest away.Fixing this properly meant giving the RPC system native map support — something we'd been working around with list-of-struct patterns (like
NamedValueandHttpHeader) but never actually had. The first commit addstype: mapwithkey/valuefields to rpcgen, so you can writetype: map, key: string, value: stringin a schema and get an idiomaticmap[string]stringin Go with proper CBOR/JSON serialization. The second commit uses that to thread attributes through the full pipeline:toLogEntry()populates them, the wire format carries them, andprintLogEntry()renders them as sortedkey=valuepairs after the message.Fixes MIR-776