fix: Export binary-tagged UTF-8 attributes over OTLP - #2259
fix: Export binary-tagged UTF-8 attributes over OTLP#2259bdewater-thatch wants to merge 1 commit into
Conversation
thompson-tomo
left a comment
There was a problem hiding this comment.
Do we not need to also make the same changes in both metrics & log exporter?
7cecb77 to
62aebce
Compare
|
Both Python (which uses native Unicode strings) and JS (via TextEncoder) inherently enforce valid text encoding, so they don't have this kind of issue. I think this is a valid fix for safely relabeling incorrectly tagged binary strings back to UTF-8. |
kaylareopelle
left a comment
There was a problem hiding this comment.
Thanks for your patience, @bdewater-thatch!
After thinking this through a bit more, I'd like to see the scope of this fix narrower than what's currently in this PR.
The original problem came from OTLP export. I recommend we focus on that problem as a first step (including the changes to Common::Utilities#utf8_encode) and consider adding more checking/normalizing to other parts of the project later on.
Adding normalization to attributes in the SDKs, Resources, etc. does make it seem safer to avoid this error from ever reaching the exporter, but this seems like a lot of changes to some stable parts of code that I'd rather consider more closely, in a separate PR, before making changes.
There are a few sections in the Common spec concepts that make it seem like we may not want do this normalization, but it requires a bit of deciphering to know for sure. Fully supporting this part of the spec may include adding some new features (like the ability to convert strings into bytes_value.
IIRC, the SDK changes came from our SIG meeting and if that's the case, I'm sorry to go back on what we discussed there. Since OTel switched to the new LFX Zoom platform, I haven't been able to find our recordings so I can't go back and check.
My request:
- Keep the changes in common/utilities
- Keep the changes in the OTLP exporters
- Remove the changes from the SDKs
Rack exposes request header values as ASCII-8BIT strings. A Cloudflare
cf-ipcityheader containingMontréaltherefore reaches the exporter (if Rack instrumentation is so configured) with valid UTF-8 bytes but a binary encoding tag. Protobuf attempts an ASCII-8BIT to UTF-8 conversion, raises Encoding::UndefinedConversionError, and the exporter sends the literal valueEncoding Error.The exporter now duplicates binary-tagged strings, relabels the duplicate as UTF-8, and uses it only when valid_encoding? succeeds. It does not mutate the instrumentation-owned value or silently scrub malformed data. This corresponds with https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/common/attribute-type-mapping.md#string-values