Add support for serializing KeyValue lists#7015
Conversation
|
Question: As far as I know, the spec doesn't say anything about limits to have many nested layers there can be, but shouldn't we try to guard against self-referencing IEnumerables? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7015 +/- ##
==========================================
- Coverage 89.98% 89.77% -0.21%
==========================================
Files 277 278 +1
Lines 14622 14778 +156
==========================================
+ Hits 13157 13267 +110
- Misses 1465 1511 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
|
I think so, otherwise it would cause a stack overflow? Could just add a recursion limit to track such occurences. |
9a4d11f to
8d385e9
Compare
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
420a92c to
22d3807
Compare
|
I added a limit to recursion depth and a unit test for the behavior. For now I arbitrarily set the limit to 3, because I'm guessing it would be subject to discussion anyway. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
martincostello
left a comment
There was a problem hiding this comment.
Could you also extend the fuzz tests to cover this functionality please?
e7ae49c to
f68b676
Compare
| from severity in Gen.Elements( | ||
| LogRecordSeverity.Debug, | ||
| LogRecordSeverity.Error, | ||
| LogRecordSeverity.Fatal, | ||
| LogRecordSeverity.Info, | ||
| LogRecordSeverity.Trace, | ||
| LogRecordSeverity.Unspecified, | ||
| LogRecordSeverity.Warn) |
There was a problem hiding this comment.
Could this be simplified using Enum.GetValues()?
There was a problem hiding this comment.
Enum.GetValues() doesn't exist on .NET Framework, so we'd have to put it behind a preprocessor directive which wouldn't be very elegant.
There was a problem hiding this comment.
It does, you just have to use the GetValues(Type) version not GetValues<T>(). But we can leave it if it's still too ugly.
|
We are having trouble sending complex nested objects to an OpenTelemetryCollector. I have tested the fork of @juliuskoval by connecting NLog.Target.OpenTelemetryProtocol with the OpenTelemetry-dotnet. I only need to traslate the object into IEnumerable<KeyValuePair<string, object?>>, and the everything works. I would kindly ask you to merge this and release new version. This would be really important for us. |
|
@SalvoDeveloper I'm waiting for at least one other approving review from one of the other maintainers before merging this. Once merged it will be part of the next release (which has no specific date at this time). After it's merged you can use consume prerelease builds from MyGet if you can't wait until then. |
|
Before merging, I'd like us to align on:
Strong support for the underlying use case, but want to avoid locking in behavior we can't revisit. |
Co-authored-by: Martin Costello <martin@martincostello.com>
…ListAttributeTests.cs Co-authored-by: Martin Costello <martin@martincostello.com>
f68b676 to
13de60b
Compare
|
@rajkumar-rangaraj @martincostello I added the benchmarks that we talked about 2 weeks ago. It measures serializing a log with a varying number of attributes and optionally a KvList attribute with no nesting and also with a varying number of attributes. BenchmarkDotNet v0.15.8, Windows 11 (10.0.26200.8457/25H2/2025Update/HudsonValley2)
|
| => maxLength.HasValue && value.Length > maxLength | ||
| ? value.Slice(0, maxLength.Value) | ||
| : value; |
There was a problem hiding this comment.
| => maxLength.HasValue && value.Length > maxLength | |
| ? value.Slice(0, maxLength.Value) | |
| : value; | |
| => maxLength is { } maxLengthValue && value.Length > maxLengthValue | |
| ? value.Slice(0, maxLengthValue) | |
| : value; |
|
These changes seem reasonable to me. Raj is on PTO until August so let's wait on another +1 from another maintainer. Can you also update the CHANGELOGs as-appropriate please? |
According to the spec, attribute values should be
AnyValue. One of the possible values ofAnyValueisKeyValueList, which we currently aren't able to serialize properly.Changes
This PR adds support for serializing KeyValue lists to the OTLP exporter.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)