fix(trace-utils): fix forward compatibility for string streaming - #2295
fix(trace-utils): fix forward compatibility for string streaming#2295anais-raison wants to merge 2 commits 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📦
|
|
![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
BenchmarksComparisonBenchmark execution time: 2026-08-06 13:32:56 Comparing candidate commit 7970c7d in PR branch Found 13 performance improvements and 17 performance regressions! Performance is the same for 107 metrics, 2 unstable metrics.
|
| /// strings inside `value` report a lied `'static` lifetime (see `Buffer::as_mut_slice`) but | ||
| /// really borrow from `owner`'s memory. | ||
| fn record_strings_in_value_ref<T: DeserializableTraceData>( | ||
| value: &rmpv::ValueRef<'static>, |
There was a problem hiding this comment.
I think this is enough, and restrict the possibility for other "lying static liftime" ?
| value: &rmpv::ValueRef<'static>, | |
| value: &rmpv::ValueRef<'_>, |
| /// Returns the underlying owned bytes buffer, unaffected by the lied `'static` lifetime | ||
| /// `as_mut_slice` exposes. Cloning this (cheap: a refcount bump for `T::Bytes = Bytes`) | ||
| /// before advancing the buffer lets callers keep the backing allocation alive for data | ||
| /// borrowed from a since-advanced position. |
There was a problem hiding this comment.
Nit: I feel this comment is a bit too specific for this function. This is just an accessor, so "Return the underlying owned bytes buffer" should be enough? The rest of the explanation is important too, but I wonder if it would rather belong to the callsite of bytes() in skip_unknown_value...
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