Add suggested string representation of complex attributes for non-OTLP protocols#4848
Conversation
6d0acbe to
b404fb8
Compare
b404fb8 to
debb47b
Compare
|
I quick checked Collector codebase and I think this matches what Collector does, which is good: https://github.com/open-telemetry/opentelemetry-collector/blob/77ffe4a370df7289fbc64c56b37eb3669fddd4ba/pdata/pcommon/value.go#L386 @open-telemetry/collector-maintainers it would be great if you can double check. I think it is nice to have Collector aligned with this too. |
Thanks @tigrannajaryan! The only difference I see there is the handling of In this PR, I propose ProtoJSON format for those values:
While the Collector Value AsString produces these string values:
|
mx-psi
left a comment
There was a problem hiding this comment.
This looks good to me and as Tigran said I believe it matches the existing AsString method in the Collector's pdata
A bit more research on the Collector:
I opened open-telemetry/opentelemetry-collector#14487 to discuss if this edge case is worth handling in the Collector (this specification PR already uses SHOULD, so it's not mandatory, and I think there's a reasonable argument to using a standard Json library over supporting these edge cases). |
### Context - Deprecate Jaeger propagator and make propagator implementation optional. ([#4827](#4827)) - Deprecate OT Trace propagator and make propagator implementation optional. ([#4851](#4851)) ### Metrics - Add normative language to the Metrics API/SDK spec concurrency requirements. ([#4868](#4868)) ### Logs - Add optional `Exception` parameter to Emit LogRecord. ([#4824](#4824)) - Add normative language to the Logging API/SDK spec concurrency requirements. ([#4885](#4885)) ### Resource - Refine the handling of OTEL_RESOURCE_ATTRIBUTES. ([#4856](#4856)) ### Common - Add string representation guidance for complex attribute value types (byte arrays, empty values, arrays, and maps) for non-OTLP protocols. ([#4848](#4848)) ### Compatibility - Stabilize Prometheus Counter to OTLP Sum transformation. ([#4862](#4862)) - Stabilize Prometheus Gauge to OTLP Gauge transformation. ([#4871](#4871)) ### SDK Configuration - Swap Tracer/Meter/LoggerConfig `disabled` for `enabled` to avoid double negatives ([#4823](#4823)) - Declarative configuration: rename `ComponentProvider` to `PluginComponentProvider`, `CreatePlugin` to `CreateComponent` in effort to use consistent vocabulary ([#4806](#4806)) - Declarative configuration: Update instrumentation config behavior to return empty object when not set ([#4817](#4817))
…open-telemetry#15138) ## Summary Update `float64AsString` to return `"NaN"`, `"Infinity"`, and `"-Infinity"` instead of `"json: unsupported value: ..."` for special float values. Fixes open-telemetry#14487 ## Problem The current `Value.AsString()` for double values containing `NaN`, `+Inf`, or `-Inf` returns strings like: ``` json: unsupported value: +Inf json: unsupported value: NaN ``` The [OpenTelemetry specification's suggested string representation](open-telemetry/opentelemetry-specification#4848) for non-OTLP protocols specifies these should be `"NaN"`, `"Infinity"`, and `"-Infinity"`. ## Fix Updated `float64AsString` in `pdata/pcommon/value.go` to return spec-compliant strings: - `math.NaN()` → `"NaN"` - `math.Inf(1)` → `"Infinity"` - `math.Inf(-1)` → `"-Infinity"` ## Test Plan - [x] Updated existing `"bad float64"` test case and added `NaN` and `-Infinity` cases - [x] Full `pdata` test suite passes with `-race`
I think the only concrete impact this has in the specification is for the Prometheus exporter. It could apply to the Zipkin exporter but that's been deprecated, so I think it's up to language implementations if they want to update their Zipkin exporter to emit complex attributes or not.