fix(otap): guard OTAP views against encoded transport IDs#3524
fix(otap): guard OTAP views against encoded transport IDs#3524yeomin4242 wants to merge 13 commits into
Conversation
github-actions
Bot
added
area:pipeline
Pull request dashboard statusStatus last refreshed: 2026-07-25 22:02:47 UTC.
This automated status or its linked feedback items may be incorrect. If something looks wrong, please report it with the result you expected. If you believe this pull request is incorrectly routed as waiting on the author, comment |
There was a problem hiding this comment.
Pull request overview
This PR prevents silently-wrong OTAP Logs/Metrics/Traces view grouping/indexing when transport-optimized (delta/quasi-delta) ID columns reach the view layer before decoding. It does so by adding centralized “fail fast” guards in the view constructors and updating key core-nodes/contrib-nodes callers to decode (or partially decode) transport-optimized IDs before constructing views.
Changes:
- Add a typed pdata error (
Error::TransportOptimizedIdsNotDecoded) plus shared helpers to validate required ID-like columns are plain-encoded before building view indexes. - Guard OTAP logs/metrics/traces view construction (and add resource-only logs view/decode helpers for routing/validation paths).
- Update affected processors/exporters to decode or preflight OTAP Arrow records before constructing views; add transport-guard tests across signals.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/otap-dataflow/crates/pdata/src/views/otap/transport_guard_test_util.rs | Test helper for forcing column-encoding metadata and asserting the new typed transport guard error. |
| rust/otap-dataflow/crates/pdata/src/views/otap/traces/transport_guard_tests.rs | New traces-focused tests ensuring encoded IDs are rejected and decoded IDs succeed. |
| rust/otap-dataflow/crates/pdata/src/views/otap/traces.rs | Add transport-ID guard before traces view indexing; wire in new tests and mark plain ID metadata in existing tests. |
| rust/otap-dataflow/crates/pdata/src/views/otap/metrics/transport_guard_tests.rs | New metrics-focused tests for guard behavior plus preflight behavior. |
| rust/otap-dataflow/crates/pdata/src/views/otap/metrics.rs | Add guard checks around metrics view construction, add “aggregatable metrics” preflight helper, and refactor temporality/monotonic reads. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs/transport_guard_tests.rs | New logs guard tests, including resource-only decode/view validation scenarios. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs/test_util.rs | Shared test utility for building fields with explicit OTAP encoding metadata. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs/resources/view.rs | Introduce resource-only OTAP logs view with plain-ID guards for resource routing/validation use cases. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs/resources/decoded.rs | Implement selective clone+decode for only resource-level IDs/attrs (including keyed filtering) to avoid full-view decoding. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs/resources.rs | Module glue and public re-exports for the new resource-only logs view APIs. |
| rust/otap-dataflow/crates/pdata/src/views/otap/logs.rs | Add fail-fast guards in OtapLogsView::new, re-export resource-only APIs, and update tests to mark ID columns plain. |
| rust/otap-dataflow/crates/pdata/src/views/otap/common.rs | Add shared “ensure plain-encoded columns” guard helpers and a distinct resource-id helper used by resource-only views. |
| rust/otap-dataflow/crates/pdata/src/views/otap.rs | Add DecodedOtapArrowRecords wrapper, shared transport-plain checks, and re-export new helpers/APIs. |
| rust/otap-dataflow/crates/pdata/src/otap/transform/transport_optimize.rs | Add remove_transport_optimized_resource_id for resource-only decode paths. |
| rust/otap-dataflow/crates/pdata/src/otap/transform.rs | Ensure empty-batch parent_id decoding marks encoding metadata as plain; add regression test. |
| rust/otap-dataflow/crates/pdata/src/error.rs | Add new typed error variant for “transport-optimized IDs not decoded”. |
| rust/otap-dataflow/crates/core-nodes/src/processors/temporal_reaggregation_processor/mod.rs | Avoid constructing metrics view unless preflight finds aggregatable metrics; decode transport IDs before view creation when needed. |
| rust/otap-dataflow/crates/core-nodes/src/processors/content_router/mod.rs | Switch routing to resource-only decode/view to avoid full logs view and to handle transport-optimized IDs safely. |
| rust/otap-dataflow/crates/core-nodes/src/exporters/console_exporter/mod.rs | Decode transport-optimized IDs before constructing OTAP logs view for console output. |
| rust/otap-dataflow/crates/contrib-nodes/src/processors/resource_validator_processor/mod.rs | Switch validation to resource-only decode/view for safe, cheaper resource inspection under transport optimization. |
| rust/otap-dataflow/crates/contrib-nodes/src/exporters/azure_monitor_exporter/exporter.rs | Decode transport-optimized IDs before OTAP logs view construction for Azure Monitor export path. |
| rust/otap-dataflow/.chloggen/otap-views-transport-id-guards.yaml | Changelog entry documenting the bug fix and new helper APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3524 +/- ##
==========================================
- Coverage 86.69% 86.67% -0.02%
==========================================
Files 788 793 +5
Lines 319377 320057 +680
==========================================
+ Hits 276871 277416 +545
- Misses 41982 42117 +135
Partials 524 524
🚀 New features to boost your workflow:
|
|
In @albertlockett’s absence, I’d like @JakeDern to review this PR. Thanks |
There was a problem hiding this comment.
Thank you for working hard on this! I see there have been a lot of iterations between this PR and the previous.
Apologies if this has already been discussed elsewhere, but I'm wondering why the solution is not to call OtapArrowRecords::decode_transport_optimized_ids in the various try_from implementations? That would be a very small change (3 lines or so) and I think solve the problem.
In my opinion view creation should not fail unless we're seeing data corruption as there's no alternative for a caller in the failure case. This PR seems to take the same opinion, as we have updated all callers to decode before creating the view to avoid it.
I'm wondering then why we're letting callers make the mistake of not decoding in a way that can only be found at runtime instead of proactively doing it in the implementation.
| } | ||
| } | ||
|
|
||
| fn metrics_root_batch( |
There was a problem hiding this comment.
I think OtapArrowRecords::root_payload_type exists for this already which can be combined with OtapArrowRecords::root_record_batch as well
| /// This reads only the root metrics batch columns needed for the decision and | ||
| /// intentionally avoids decoding transport-optimized child IDs or constructing | ||
| /// hierarchy indexes. | ||
| pub fn otap_metrics_have_aggregatable_metrics(records: &OtapArrowRecords) -> Result<bool, Error> { |
There was a problem hiding this comment.
Not sure that the views module is the appropriate place to put this function, and it seems to repeat much of the logic in the temporal_reaggregation_processor for the same thing.
| log_attrs: Option<&'a RecordBatch>, | ||
| ) -> Result<Self, Error> { | ||
| if let Some(batch) = logs_batch { | ||
| ensure_plain_encoded_columns( |
There was a problem hiding this comment.
Apologies if this has already been litigated elsewhere, but I'm wondering if raising an error is the right behavior here instead of decoding the transport optimized IDs and then continuing. Is there a reason we want to force the caller to do it?
Awareness of cost is certainly one reason to have the API this way, but I would imagine if a caller needed to create a view there's not really a good way to handle this error. IMO the only reason view creation should fail is corrupt data.
| metrics_payload_type: ArrowPayloadType, | ||
| metrics_batch: &arrow::array::RecordBatch, | ||
| ) -> Result<(), Error> { | ||
| ensure_plain_encoded_columns(metrics_payload_type, metrics_batch, ROOT_ID_COLUMNS)?; |
There was a problem hiding this comment.
If we need to understand the relationships between payload types, I think we should be using code like payload_relations or the payload schema definitions in
| const ROOT_ID_COLUMNS: &[&str] = &[consts::ID, RESOURCE_ID_COL_PATH, SCOPE_ID_COL_PATH]; | ||
| const PARENT_ID_COLUMNS: &[&str] = &[consts::PARENT_ID]; | ||
| const ID_AND_PARENT_ID_COLUMNS: &[&str] = &[consts::ID, consts::PARENT_ID]; |
There was a problem hiding this comment.
Similar feedback here, this information is available elsewhere
Thanks for raising this. I agree that callers should not have to remember to decode first, and that this error should not normally surface in application code. The current TryFrom<&OtapArrowRecords> implementation only has immutable access, whereas decoding requires mutable access. Cloning within the implementation would not work either, because the returned view borrows from the records and the clone would not live long enough. I suggest keeping the guard so that direct constructors cannot silently build incorrect indexes, while adding a mutable entry point that decodes the records and creates the view in one step. For callers with only immutable access, a decoded wrapper can own the decoded records and keep them alive for as long as the view is needed. Would that address your concern? |
Thank you for pointing out the mutable access requirement, it's definitely important. I also agree that we can't allow callers to silently build invalid views and all available APIs have to result in either valid construction or an error. I'm currently on mobile and not able to check, so apologies if this is incorrect, but think all current callers will have ownership and we should provide an API that leverages this fact. That could mean changing the try_from implementation to take an exclusive reference, or, if that feels awkward, to replace it with a separate api directly on the concrete view types and update all callers to use them. In either case I still think the solution is to proactively decode the ids for the caller and eliminate this mode of fallibility. If there are callers that don't have that access then I think we should look at that separately, but IMO proactive decoding is very simple/effective and should be the first choice. |
Change Summary
Problem
OtapLogsView,OtapMetricsView, andOtapTracesViewbuild grouping and childindexes from OTAP Arrow ID columns. If transport-optimized records reach these
views before IDs are decoded, the views can read encoded
id,resource.id,scope.id, orparent_idcolumns as plain IDs.Solution
Add fail-fast guards before each OTAP view builds indexes, and decode
transport-optimized OTAP Arrow records in callers that can receive them before
constructing the view.
Changes:
Error::TransportOptimizedIdsNotDecoded.This covers Metrics and Traces in the same PR because they share the same failure
mode as Logs, and the issue discussion called out fixing the remaining views as
acceptable.
What issue does this PR close?
How are these changes tested?
cargo fmt --allcargo test -p otap-df-pdata views::otap --libcargo test -p otap-df-pdata test_metrics_all_fields_otap_view --libcargo test -p otap-df-pdata test_traces_all_fields_otap_view --libcargo check -p otap-df-pdatacargo check -p otap-df-core-nodes --features otap-df-otap/crypto-ringcargo xtask checkAre there any user-facing changes?
No external API or configuration changes. OTAP views now return a typed error
instead of constructing over still-encoded transport ID columns.