fix: [exporter-etw] Enforce depth limit when JSON serializing complex types

[utpilla]

Addresses #234 (comment)

Changes

• Enforce a maximum depth limit of 1 when serializing AnyValue::ListAny and AnyValue::HashMap
• This means that we would correctly serialize a vec or a HashMap containing primitive types
• If a vec or a HashMap contains yet another vec or a HashMap we would serialize an error message string instead of parsing it futher

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 98.71795% with 1 line in your changes missing coverage. Please review.

Project coverage is 50.2%. Comparing base (c76b69f) to head (0151b4a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-etw-logs/src/logs/converters.rs	98.6%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #238     +/-   ##
=======================================
- Coverage   50.3%   50.2%   -0.1%     
=======================================
  Files         60      60             
  Lines       7651    7640     -11     
=======================================
- Hits        3851    3840     -11     
  Misses      3800    3800             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[cijothomas]

@psandana Could you review? This seems safer

[lalitb]

nit - should we return more structured error message. Eg

 json!({ "truncated": true, "type": "list/map", reason: " max depth reached"})

[psandana]

First. I must mention that I'm not a fan of silent failures. I would rather the user gets a panic right away, than unexpectedly finding truncated content in production logs. But I recognize that a proper fix may not come easily, thus, let's move forward.

I +1 @lalitb comment, and we should actually mention this is not an error, is a truncation. I would add the truncation depth as another value. In the future, we could allow users to modify this number under their own risk.

 json!({ "truncated": true, "type": "list/map", reason: "max depth reached", reason_depth: "1"})

[utpilla]

First. I must mention that I'm not a fan of silent failures. I would rather the user gets a panic right away, than unexpectedly finding truncated content in production logs.

I would have to disagree here. Do we really want a log statement to be capable of bringing down the entire application and affect the user's SLA and business? In my opinion, it's better that we give the user a chance to fix their telemetry without crashing their application.

we should actually mention this is not an error, is a truncation. I would add the truncation depth as another value. In the future, we could allow users to modify this number under their own risk.

This is what the message looks like now: Message truncated as nested lists/maps are not supported. I think this is much simpler for the user to understand than mentioning depth = 1 etc. We can always include depth information later, if in future we decide to support nested structures. But for now, I favor the simplicity of the current message.

[cijothomas]

+1, we can't panic. OTel spec has been very clear on this from day 1:
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/error-handling.md

[psandana]

Do we really want a log statement to be capable of bringing down the entire application and affect the user's SLA and business?

Personally, yes, I would like to fail early. Any piece of code should be covered already by tests, so I would get the panic early anyway. But I just agree is not the way that OTel specification is, so granted :)

[utpilla]

Personally, yes, I would like to fail early. Any piece of code should be covered already by tests, so I would get the panic early anyway.

I think this works well for high-level applications/libraries that have controlled and known usage. However, libraries meant for telemetry, serialization, etc. are more foundational. For these base libraries, it's crucial to be secure by default rather than relying on user testing.

Consider Newtonsoft.Json as an example: when a similar CVE was reported, they had to deprecate all previous vulnerable versions and fix the issue themselves. They couldn't simply make this the user's responsibility.

Foundational libraries need to handle edge cases robustly since they're often used in security-sensitive contexts and by developers who may not anticipate all potential attack vectors. This is probably why OpenTelemetry has this guidance.

[psandana]

to be secure by default
I've seen two meanings for the statement above: fail early, or fallback safely. I rather to fail than to produce a wrong and unexpected side effect.

With this in mind, I was thinking that the correct approach may be to:

1. Issue a warning that depth > n has been reached, and application may crash, with n default to 1.
2. Enable a feature to truncate the depth logged if a user-defined depth is trespassed
3. Just let the app crash if SO happen. Warnings have been given, and user must configure the library correctly as in previous point.

I think logs nesting dynamically just won't happen. Mostly logs structure will be statically defined. Thus, unexpected SO should be really infrequent.