Conversation
There was a problem hiding this comment.
Pull request overview
Adds log-type-aware workflow routing to avoid evaluating workflows that cannot match an incoming log.
Changes:
- Adds conservative matcher log-type inference.
- Introduces and integrates a workflow candidate router.
- Adds routing and integration tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
bd-workflows/src/workflow.rs |
Computes routes from active workflow state. |
bd-workflows/src/test.rs |
Supports typed test logs. |
bd-workflows/src/routing.rs |
Implements candidate routing. |
bd-workflows/src/routing_test.rs |
Tests route selection and refreshes. |
bd-workflows/src/lib.rs |
Registers the routing module. |
bd-workflows/src/engine.rs |
Integrates routing into event processing. |
bd-workflows/src/engine_test.rs |
Tests routing across workflow transitions. |
bd-workflows/src/engine_test_helpers.rs |
Propagates test log types. |
bd-workflows/src/config.rs |
Aggregates configured log types. |
bd-log-matcher/src/matcher.rs |
Adds log-type sets and matcher inference. |
bd-log-matcher/src/matcher_test.rs |
Tests type sets and inference. |
c0456d0 to
50f18f3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
bd-workflows/src/routing.rs:186
Vec::reservetakes an additional count relative tolen, not a target-capacity delta. Because these vectors are cleared before this helper runs, growing from (for example) capacity 4 to 6 callsreserve(2), which can do nothing and later forcescandidate_indicesto allocate while selecting a log. Reserve from the current length soprepareactually preserves the documented allocation-free processing path.
values.reserve(capacity - values.capacity());
mattklein123
left a comment
There was a problem hiding this comment.
How can we understand the perf improvment or not for this in different cases? I think you added a bench can we run it against this change in different scenarios and show the deltas?
| ); | ||
| } | ||
| }, | ||
| WorkflowEvent::SessionStart(_) | WorkflowEvent::StateChange(..) => { |
There was a problem hiding this comment.
At least for state change can't we also filter based on whether a workflow cares about it or not? Not sure about SessionStart but I think in that case we could do it also?
There was a problem hiding this comment.
Yeah i did this, it was a pretty easy extension
| // TODO(snowp): We should be able to apply this routing logic to other event types to dramatically | ||
| // reduce the number of workflows attempted for state events. For now state events are rare so we | ||
| // limit this to logs. |
There was a problem hiding this comment.
I commented on this above.
| /// This is a derived view over the engine's primary workflow vector: every stored index refers to | ||
| /// the workflow at that same position. The caller must rebuild it after adding, removing, or | ||
| /// reordering workflows; between those boundaries, the router updates only workflows which | ||
| /// processed the current log. |
There was a problem hiding this comment.
nit: not processed, but actual advanced 1 or more traversals?
| .unwrap_or_default() | ||
| }); | ||
| } else { | ||
| Self::rebuild_log_routes(log_router, workflows, configs); |
There was a problem hiding this comment.
Per other comments would be nice to now do this always for the other events, even if rare.
| configs, | ||
| stats, | ||
| needs_state_persistence, | ||
| log_router, |
There was a problem hiding this comment.
It seems a little fragile to me that we mutate the router in place and use that mutation for rebuilds, but then could theoretically early exit. Can we somehow debug_assert at least that we finalize anything that should be finalized in a single pass?
| log_router.refresh_selected_routes(|index| { | ||
| workflows | ||
| .get(index) | ||
| .zip(configs.get(index)) | ||
| .map(|(workflow, config)| workflow.log_route(config)) | ||
| .unwrap_or_default() | ||
| }); |
There was a problem hiding this comment.
This is not cheap even in the case where nothing advanced. Can we improve the performance of the very common no advance case?
There was a problem hiding this comment.
I tried this out and it seems like for the large corpus that adding an additional value that we track whether we can safely skip refresh ends up being more expensive than just doing it. Tracking the additional value added ~5% over just doing the route rebuild
|
Running it against the internal corpus I got very good results: This is primarily from this being a low to zero match session. I'll figure out how to include this larger corpus and we could formalize a way to report on workflow engine changes |
2eb4522 to
9bb1692
Compare
|
/benchmark workflows |
Instead of always evaluating all workflows for all logs, this tracks a running set of candidate workflows based on the log type of the log. This allows us to bypass workflows whose next match transitions require a different log type than the inbound log
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Route logs by inferred log type and route state changes and session starts to workflows whose active transitions can consume them. Keep the index-based router synchronized with the workflow vector, refresh only selected routes, and cover bucket updates and fallback behavior. Co-Authored-By: GPT-5 <codex@openai.com>
Pin the oversized async log buffer future in the test task to satisfy Clippy large_futures. Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
Co-Authored-By: GPT-5 <codex@openai.com>
9bb1692 to
8f91cab
Compare
|
/benchmark workflows |
Benchmark:
|
| Benchmark | Absolute | Percent |
|---|---|---|
workflow_replay/large |
+105.86 ms | +38.39% |
workflow_replay/small |
+1.22 µs | +10.02% |
The Criterion HTML report is attached to this workflow run.
Instead of always evaluating all workflows for all logs, this tracks a running set of candidate workflows based
on the log type of the log. This allows us to bypass workflows whose next match transitions require a different
log type than the inbound log.
We keep a router table that provides a routing layer on top of the simple workflow list: for workflows that can be determined to require a particular log type we can filter down the considered workflows to workflows constrained to a log type + all ambiguous workflows. This is kept up to date during workflow execution, rebuilding the routing rules as the list of active runs.
For now this is only going to apply to initial conditions due to not working with the run timeout defined on most workflows, but the initial run ends up being a very significant part of the workflows considered in practice.
Fixes BIT-9477