Serialize additional dictionary shapes as KeyValue lists - #7679
martincostello merged 6 commits into
Conversation
|
Welcome, contributor! Thank you for your contribution to opentelemetry-dotnet. Important reminders:
|
Pull request dashboard statusMerged · refreshed 2026-08-28 16:11 UTC Status above doesn't look right?
|
d48a27b to
1cdcc82
Compare
|
/easycla |
martincostello
left a comment
There was a problem hiding this comment.
Just some minor comments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7679 +/- ##
==========================================
- Coverage 91.55% 91.52% -0.03%
==========================================
Files 321 321
Lines 17982 17988 +6
==========================================
+ Hits 16463 16464 +1
- Misses 1519 1524 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Simplify the CHANGELOG entries and drop the explanatory comments called out in review.
23cdf7c to
23f7785
Compare
|
Do you have an example of the after to compare to the before in #7678 just to visualise the difference? |
|
Also while you're touching the file, this area might also be a candidate for similar optimisations as in #7681 when iterating the KV pairs. |
|
Could you extend even further to support |
…d-kvlist-support # Conflicts: # src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
|
@martincostello good shout on the #7681-style optimisation — I had a go at it. I've pushed a commit that swaps the The honest caveat is it grows the PR a fair bit — there's a new @thomhurst the The nearest shape that does compile is |
Yeah, I think this is a lot bigger a change than I expected (compared to ~70 LoC in #7681). Let's revert that for now, and you can resubmit post merge if you're interested. I would however avoid changes to Zipkin where possible as it's deprecated and will be removed at the end of this year. Small changes to the console exporter are fine, but there's no need for that to eek every ounce of allocation out of it. Also still waiting on #7679 (comment). |
This reverts commit 9134d21.
…d-kvlist-support # Conflicts: # src/Shared/TagWriter/TagWriter.cs
|
Example of before and after: logger.LogInformation("Processed order {Order} with labels {Labels}",
new Dictionary<string, object?> { ["id"] = "ord_123", ["total"] = 42.5 },
new Dictionary<string, string> { ["region"] = "eu", ["env"] = "prod" });Above Before { "key": "Labels", "value": { "stringValue": "System.Collections.Generic.Dictionary`2[System.String,System.String]" } }After {
"key": "Labels",
"value": {
"kvlistValue": {
"values": [
{ "key": "region", "value": { "stringValue": "eu" } },
{ "key": "env", "value": { "stringValue": "prod" } }
]
}
}
} |
|
Cool thanks - do the changes here also resolve the example as described in #6035? |
Fixes #7678
Changes
TagWriter: pulled the depth-guarded kvlist path from Add support for serializing KeyValue lists #7015 intoTryWriteKvListTagWithinDepthLimit, then added two cases,IEnumerable<KeyValuePair<string, string?>>(e.g.Dictionary<string, string>) and non-genericIDictionary(e.g.Dictionary<string, int>,Hashtable). Both sit after theArraycase so the existing type checks are untouchedZipkinTagWriter: kvlist values are now a JSON object embedded in a string, the same treatment arrays already get, rather thanConvert.ToString()outputHashtablewith non-string keys and the depth-limit fallback) and CHANGELOG entriesBenchmark before/after (
ProtobufOtlpLogSerializerBenchmarks, net9.0, Apple M-series,--job short) shows no measurable change. The new checks only run for values that previously fell through toConvert.ToString().Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)🤖 Generated with Claude Code