fix(trace-utils): fix forward compatibility for string streaming - #2295
fix(trace-utils): fix forward compatibility for string streaming#2295anais-raison wants to merge 1 commit into
Conversation
…1 decoder + add tests
📚 Documentation Check Results📦
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
🔒 Cargo Deny Results📦
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: ac8e79e | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac8e79eff6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| match value { | ||
| rmpv::ValueRef::String(s) => { | ||
| if let Some(s) = (*s).into_str() { | ||
| table.record(&T::Text::from_static_str(s)); |
There was a problem hiding this comment.
When decoding owned bytes, this records a string borrowed from the just-skipped buffer as from_static_str, so if a future/unknown field contains the first occurrence of a string and a later known field references that ID, the decoded payload can contain a BytesString that does not retain the original Bytes allocation. In the common from_bytes(Bytes::from(Vec<_>)) path, a payload like unknown key "ghost" followed by span service ref 1 leaves service pointing at memory owned only by the decoder buffer after it is dropped; this should record skipped strings via a path that preserves the backing Bytes ownership (or copies) rather than treating them as static.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This needs to be addressed. The 'static lifetime is actually a lie since the string you're getting is actually a reference to the Bytes buffer, to make sure the Bytes refcount is correctly updated you need to create a ByteString from the slice with from_bytes_slice
There was a problem hiding this comment.
By the way, how it's possible in safe code? I haven't looked at record or from_static_str, but should one of the two be marked as unsafe if there are additional pre-conditions to hold? As this PR shows, this seems to be actually possible to hit.
There was a problem hiding this comment.
Ok, I just looked and the culprit is as_mut_slice from the decoding buffer. Why does this method return a &'static?? It's obviously not, if T::Bytes is owning memory... Honestly we're very lucky Codex spotted this. I would have written the same code as @anais-raison here without giving it a second thought and would probably have never find that at review time either. Something went really wrong with the as_mut_slice design/implementation
|
Code lgtm but the tests are hard to read, and it might be over-tested. (I don't think we need 1 test per field) |
(This is my fault lol as I originally provided these tests for this ticket after a quick claude generation and didn't clean them up to be PR ready before sharing them :P Sorry!) |
| match value { | ||
| rmpv::ValueRef::String(s) => { | ||
| if let Some(s) = (*s).into_str() { | ||
| table.record(&T::Text::from_static_str(s)); |
There was a problem hiding this comment.
This needs to be addressed. The 'static lifetime is actually a lie since the string you're getting is actually a reference to the Bytes buffer, to make sure the Bytes refcount is correctly updated you need to create a ByteString from the slice with from_bytes_slice
5 participants
What does this PR do?
Fixes a forward-compatibility bug in the v1 msgpack decoder: when skipping an unknown key (e.g. a field added by a newer tracer version), inline strings inside the skipped value were not interned into the shared
StringTable. This desynced the table's indices, so any later reference to a previously-interned string by ID would resolve to the wrong value.Motivation
APMSP-3836