Improve transform chain naming 337#1990
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Made-with: Cursor
3105d3c to
853708a
Compare
There was a problem hiding this comment.
Pull request overview
This PR makes transform instance naming mandatory and threads those names through chain/subchain construction, validation, and runtime error context so logs/metrics/errors can uniquely identify specific configured instances.
Changes:
- Add mandatory
nameto transform configs and propagate it through chain/subchain naming (including derived names likeParallelMap’s{name}[i]). - Add recursive topology name uniqueness validation across sources, transforms, and chains (including subchains).
- Decouple
QueryCounter’s transform instance name from theshotover_query_count{name=...}label viacounter_name, and include chain context in transform failure messages.
Reviewed changes
Copilot reviewed 128 out of 128 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| shotover/src/transforms/valkey/sink_single.rs | Add mandatory transform instance name. |
| shotover/src/transforms/valkey/sink_cluster.rs | Add mandatory transform instance name. |
| shotover/src/transforms/valkey/cluster_ports_rewrite.rs | Add mandatory transform instance name. |
| shotover/src/transforms/valkey/cache.rs | Add name, derive cache subchain name, adjust tests. |
| shotover/src/transforms/throttling.rs | Add name to config and update tests. |
| shotover/src/transforms/tee.rs | Add name, restructure mismatch subchain config, derive chain names, update tests. |
| shotover/src/transforms/query_counter.rs | Add counter_name; use it for query-count metric labeling. |
| shotover/src/transforms/parallel_map.rs | Add name, derive {name}[i] subchain names, update tests. |
| shotover/src/transforms/opensearch/mod.rs | Add mandatory transform instance name. |
| shotover/src/transforms/null.rs | Convert to named config struct and implement naming. |
| shotover/src/transforms/mod.rs | Extend TransformConfig trait; add chain name to error context. |
| shotover/src/transforms/load_balance.rs | Name subchain using transform name. |
| shotover/src/transforms/kafka/sink_single.rs | Add mandatory transform instance name. |
| shotover/src/transforms/kafka/sink_cluster/mod.rs | Add mandatory transform instance name. |
| shotover/src/transforms/filter.rs | Add name to config; update tests for new chain reset API. |
| shotover/src/transforms/debug/returner.rs | Add mandatory transform instance name. |
| shotover/src/transforms/debug/printer.rs | Convert to named config struct and implement naming. |
| shotover/src/transforms/debug/log_to_file.rs | Convert to named config struct and implement naming. |
| shotover/src/transforms/debug/force_parse.rs | Add mandatory name to debug force parse/encode configs. |
| shotover/src/transforms/coalesce.rs | Add name and update tests for new chain reset API. |
| shotover/src/transforms/chain.rs | Track per-transform instance names; pass chain name into ChainState. |
| shotover/src/transforms/cassandra/sink_single.rs | Add mandatory transform instance name. |
| shotover/src/transforms/cassandra/sink_cluster/mod.rs | Add mandatory transform instance name. |
| shotover/src/transforms/cassandra/peers_rewrite.rs | Add mandatory transform instance name. |
| shotover/src/sources/mod.rs | Expose chain config getter for recursive validation. |
| shotover/src/config/topology.rs | Implement recursive name uniqueness validation and expand tests. |
| shotover/src/config/chain.rs | Build chains using (builder, user-name) pairs. |
| shotover/benches/benches/chain.rs | Update benches to supply transform instance names. |
| shotover-proxy/tests/valkey_int_tests/mod.rs | Update expected error strings to include chain + instance names. |
| shotover-proxy/tests/valkey_int_tests/basic_driver_tests.rs | Update expected error strings to include chain + instance names. |
| shotover-proxy/tests/test-configs/valkey/tls/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/tls/topology-encode.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/tls-no-verify-hostname/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/tls-no-client-auth/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/passthrough/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/passthrough/topology-encode.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-tls/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-tls/topology-no-source-encryption.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-tls/topology-encode.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-ports-rewrite/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-hiding/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-hiding/topology-encode.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-handling/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/valkey/cluster-dr/topology.yaml | Add required name fields; add counter_name. |
| shotover-proxy/tests/test-configs/valkey/cluster-auth/topology.yaml | Add required name for transforms. |
| shotover-proxy/tests/test-configs/tee/switch_chain.yaml | Add required name fields; update mismatch subchain structure. |
| shotover-proxy/tests/test-configs/tee/subchain_with_mismatch.yaml | Add required name fields; update mismatch subchain structure. |
| shotover-proxy/tests/test-configs/tee/subchain.yaml | Add required name fields; update mismatch subchain structure. |
| shotover-proxy/tests/test-configs/tee/log_with_mismatch.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/tee/log.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/tee/ignore_with_mismatch.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/tee/ignore.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/tee/fail_with_mismatch.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/tee/fail.yaml | Add required name fields. |
| shotover-proxy/tests/test-configs/query_type_filter/simple.yaml | Add required name fields for transforms. |
| shotover-proxy/tests/test-configs/opensearch-passthrough/topology.yaml | Add required name for OpenSearch sink. |
| shotover-proxy/tests/test-configs/null-valkey/topology.yaml | Add transform name and QueryCounter.counter_name. |
| shotover-proxy/tests/test-configs/null-cassandra/topology.yaml | Add required name for NullSink. |
| shotover-proxy/tests/test-configs/log-to-file/topology.yaml | Add required name fields for transforms. |
| shotover-proxy/tests/test-configs/kafka/single-sasl-scram-plaintext-source-tls-sink/topology.yaml | Add required name for Kafka sink. |
| shotover-proxy/tests/test-configs/kafka/passthrough/topology.yaml | Add required name for Kafka sink. |
| shotover-proxy/tests/test-configs/kafka/passthrough/topology-encode.yaml | Add required name fields for transforms. |
| shotover-proxy/tests/test-configs/kafka/passthrough-tls/topology.yaml | Add required name for Kafka sink. |
| shotover-proxy/tests/test-configs/kafka/passthrough-sasl-scram/topology.yaml | Add required name for Kafka sink. |
| shotover-proxy/tests/test-configs/kafka/passthrough-sasl-plain/topology.yaml | Add required name fields for transforms. |
| shotover-proxy/tests/test-configs/kafka/passthrough-mtls/topology.yaml | Add required name for Kafka sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-tls/topology.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-scram/topology-single.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-scram-over-mtls/topology3.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-scram-over-mtls/topology2.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-scram-over-mtls/topology1.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-scram-over-mtls/topology-single.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-plain/topology3.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-plain/topology2.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-plain/topology1.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-sasl-plain/topology-single.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-mtls/topology.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-3-racks/topology-rack3.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-3-racks/topology-rack2.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-3-racks/topology-rack1.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-2-racks/topology-rack2.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-2-racks/topology-rack1.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-1-rack/topology3.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-1-rack/topology2.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-1-rack/topology1.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/kafka/cluster-1-rack/topology-single.yaml | Add required name for Kafka cluster sink. |
| shotover-proxy/tests/test-configs/invalid_terminating_not_last.yaml | Update invalid config to include required transform names. |
| shotover-proxy/tests/test-configs/invalid_subchains.yaml | Update invalid config to include required transform/subchain names. |
| shotover-proxy/tests/test-configs/invalid_protocol_mismatch.yaml | Update invalid config for QueryCounter.counter_name + names. |
| shotover-proxy/tests/test-configs/invalid_non_terminating_last.yaml | Update invalid config to include required transform name. |
| shotover-proxy/tests/test-configs/hotreload/topology.yaml | Update hotreload configs for required names + counter_name. |
| shotover-proxy/tests/test-configs/hotreload/topology-tls-old.yaml | Update hotreload configs for required names + counter_name. |
| shotover-proxy/tests/test-configs/hotreload/topology-tls-new.yaml | Update hotreload configs for required names + counter_name. |
| shotover-proxy/tests/test-configs/hotreload/topology-alt.yaml | Update hotreload configs for required names + counter_name. |
| shotover-proxy/tests/test-configs/cassandra/valkey-cache/topology.yaml | Add required names for cache + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/tls/topology.yaml | Add required name for Cassandra sink. |
| shotover-proxy/tests/test-configs/cassandra/tls/topology-with-key.yaml | Add required name for Cassandra sink. |
| shotover-proxy/tests/test-configs/cassandra/request-throttling/topology.yaml | Add required names for throttling + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/request-throttling.yaml | Add required names for throttling + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/peers-rewrite/topology.yaml | Add required names for peers rewrite + sinks. |
| shotover-proxy/tests/test-configs/cassandra/passthrough/topology.yaml | Add required name for Cassandra sink. |
| shotover-proxy/tests/test-configs/cassandra/passthrough/topology-encode.yaml | Add required names for encode + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/passthrough-parse-response/topology.yaml | Add required names for parse + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/passthrough-parse-request/topology.yaml | Add required names for parse + sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v5/topology.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v4/topology.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v4/topology-encode.yaml | Add required names for encode + cluster sink transforms. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v4/topology-dummy-peers.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v3/topology.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-v3/topology-dummy-peers.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-tls/topology.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack/topology_rack3.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack/topology_rack2.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack/topology_rack1.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack-2-per-rack/topology_rack3.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack-2-per-rack/topology_rack2.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cluster-multi-rack-2-per-rack/topology_rack1.yaml | Add required name for Cassandra cluster sink. |
| shotover-proxy/tests/test-configs/cassandra/cassandra-5/topology.yaml | Add required name for Cassandra sink. |
| shotover-proxy/tests/runner/runner_int_tests.rs | Update expected validation errors to use instance names. |
| shotover-proxy/tests/kafka_int_tests/mod.rs | Update expected error regex to include instance + chain context. |
| shotover-proxy/tests/cassandra_int_tests/mod.rs | Update expected error strings to include instance + chain context. |
| shotover-proxy/config/topology.yaml | Update example config to include required transform names. |
| shotover-proxy/benches/windsock/valkey/bench.rs | Add required transform names in generated topology. |
| shotover-proxy/benches/windsock/kafka/bench.rs | Add required transform names in generated topology. |
| shotover-proxy/benches/windsock/cassandra/bench.rs | Add required transform names in generated topology. |
| docs/src/transforms.md | Document mandatory naming and update examples (counter_name, subchains). |
| custom-transforms-example/src/valkey_get_rewrite.rs | Update custom transform example for new naming/subchain trait methods. |
| custom-transforms-example/config/topology.yaml | Add required transform names to example topology. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Since we no longer have any TransformConfig's without any fields, I think we can actually remove this function entirely: Might be worth keeping in case, we ever change our mind on the naming approach. |
rukai
left a comment
There was a problem hiding this comment.
I am happy with the PR at a high level, but havent given it the usual scrutiny that I would due to time constraints.
But also, if we intend to avoid changing the transform component of metrics, it does leave us in a bit of a funny position where the naming scheme is now different between metrics and logs.
But as long as changing metrics is something we want to do "eventually", then I think this PR is still a step in the right direction.
closes #337
This change makes transform instance naming mandatory and uses those names as the primary observability key across the topology. It adds uniqueness validation for all names per type (sources, chains, transforms, and subchains), so logs, errors, and metrics can point to a specific instance without ambiguity. Subchain names are now derived from the parent transform name (ParallelMap appends an index), and runtime errors include both transform and chain context for faster triage. QueryCounter decouples the transform instance name from the metrics label via counter_name, keeping metric names stable while allowing explicit labeling.
Observability impact
Metric names are unchanged, but shotover_chain_* label values will reflect derived subchain names (e.g., my-parallel-map[0], my-tee) instead of fixed internal names.
shotover_query_count{name=...} now uses counter_name rather than the transform’s name.
shotover_transform_* labels remain based on transform type (e.g., QueryCounter, NullSink).