Rust decoder shouldn't assume root payload is at position 0#2468
Rust decoder shouldn't assume root payload is at position 0#2468simathih wants to merge 1 commit intoopen-telemetry:mainfrom
Conversation
|
|
simathih
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2468 +/- ##
==========================================
- Coverage 88.18% 88.17% -0.01%
==========================================
Files 604 604
Lines 214589 214587 -2
==========================================
- Hits 189232 189214 -18
- Misses 24831 24847 +16
Partials 526 526
🚀 New features to boost your workflow:
|
|
@JakeDern could you review this PR. Thanks |
| Err(Error::RecordBatchNotFound { | ||
| payload_type: expected, | ||
| }) | ||
| } |
There was a problem hiding this comment.
nit - this can be simplified a bit to be more idiomatic Rust:
if records.arrow_payloads.iter().any(|payload| {
ArrowPayloadType::try_from(payload.r#type) == Ok(expected)
}) {
Ok(())
} else {
Err(Error::RecordBatchNotFound {
payload_type: expected,
})
}
|
Welcome, and thanks for the first PR.
|
| self.proto_buffer.clear(); | ||
| self.logs_proto_encoder | ||
| .encode(&mut otap_batch, &mut self.proto_buffer)?; | ||
| check_payload_type_present(records, ArrowPayloadType::Logs)?; |
There was a problem hiding this comment.
I don't think we need a separate ahead of time check for the root payload time. Most things are checked by from_record_messages which will fail if any of the records are invalid for the signal or have a misaligned schema.
It currently does not fail if there's no root record batch for the signal (perhaps something we should consider adding), so I think we just need to check that after we create the otap batch that otap_batch.get(ArrowPayloadType::Logs).is_some()
There was a problem hiding this comment.
(Similar feedback goes for the other signals as well)
There was a problem hiding this comment.
I'm confused, at this point. @JakeDern are you suggesting that we modify from_record_messages() to check for the root payload itself?
There was a problem hiding this comment.
@jmacd My main feedback for this PR specifically is that we don't need to pre-scan the array for the Logs payload type, rather we can call from_record_messages::<Logs>() which will screen out invalid payload types for the signal. Then it's a quick O(1) check for the Logs payload type using get(). That covers all the behavior we have today.
Beyond this PR, I'm also raising the point that we should enforce that the root payload is always present in a more central way. Modifying from_record_messages to check for something like T::root_payload_type() (which would have to be added on OtapBatchStore) is probably better than doing the check in the decoder code but isn't a full solution to the problem as we have other paths for construction and modification i.e. set and remove APIs.
There was a problem hiding this comment.
Also something I did a bit ago that I hope enables a unified path for this is splitting out the concept of a RawBatchStore which is basically just "const size storage for a collection of record batches": https://github.com/open-telemetry/otel-arrow/blob/37df685b98ec6e7edc1f0ed84d4d00c72c371bf7/rust/otap-dataflow/crates/pdata/src/otap/raw_batch_store.rs.
With that we could lock down creation of Logs to just the TryFrom<RawLogsStore> implementation and enforce spec compliance at that boundary. Then from_record_messages would collect into a RawLogsStore and then just call the TryFrom implementation. But I'm sure we'll have to touch a lot of callers to lock that down 🙂
5 participants
Change Summary
Rust decoder shouldn't assume root payload is at position 0.
We know what root payload we're expecting so we can update the code to fill in the appropriate Logs/Metrics/Traces construct and then check for root payload presence at the end.
What issue does this PR close?
#2363
How are these changes tested?
Are there any user-facing changes?