-
Notifications
You must be signed in to change notification settings - Fork 66
[kql_processor] Experimental KQL recordset processor #1730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[kql_processor] Experimental KQL recordset processor #1730
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1730 +/- ##
==========================================
+ Coverage 84.08% 84.19% +0.11%
==========================================
Files 467 473 +6
Lines 137359 137454 +95
==========================================
+ Hits 115500 115735 +235
+ Misses 21325 21185 -140
Partials 534 534
🚀 New features to boost your workflow:
|
rust/otap-dataflow/crates/otap/src/experimental/recordset_kql_processor/README.md
Show resolved
Hide resolved
rust/otap-dataflow/crates/otap/src/experimental/recordset_kql_processor/README.md
Show resolved
Hide resolved
rust/otap-dataflow/crates/otap/src/experimental/recordset_kql_processor/processor.rs
Outdated
Show resolved
Hide resolved
rust/otap-dataflow/crates/otap/src/experimental/recordset_kql_processor/processor.rs
Outdated
Show resolved
Hide resolved
rust/otap-dataflow/crates/otap/src/experimental/recordset_kql_processor/processor.rs
Outdated
Show resolved
Hide resolved
| effect_handler | ||
| .notify_nack(NackMsg::new( | ||
| message, | ||
| OtapPdata::new(ctx, OtapPayload::empty(SignalType::Logs)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should nack not send the data back if the previous component expects it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my understanding of notify_nack and notify_ack, yes, we would normally want to return original payload along with the nack. However, payload is being consumed in the conversion to OtlpProtoBytes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The design of the Ack/Nack system allows for simple components that pass through the Context to do nothing and it will just work.
There is not much to worry about with dropping the input data, unless we want to use the current retry processor before the KQL processor. As long as we retry after KQL processor, not a problem.
Relates to #1642
Changes
Details
@drewrelmas took @jmacd's original work and added tests + config. I cleaned it up a bit and improved the bridge API to allow the processor to own the pipeline memory instead of storing it in a static. The static path is in place for callers needing to invoke things using FFI (from non-Rust platforms).