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?
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?