chore(core): unify how we define identifiers for subsystems/components - #2029
Conversation
dd-octo-sts
Bot
added
area/core
Binary Size Analysis (Agent Data Plane)Baseline: 4c0e98b · Comparison: d4129ca · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
tobz
added
the
type/chore
d16e35a to
3cad967
Compare
2ee39ae to
65f7e30
Compare
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for a single canonical identifier for “components/subsystems” so health registration, resource-accounting paths, and supervisor/process-tree names stay byte-identical (starting with topology components).
Changes:
- Introduces
SubsystemIdentifierand a sharedtopology_root()to derive canonical dotted identifiers (topology.<name>...) across subsystems. - Updates topology component contexts, health registration, supervision naming, and resource-accounting registry paths to use the canonical identity.
- Adjusts runtime process-name sanitization to preserve dotted scoping and updates tests/usages across crates accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/saluki-core/src/topology/mod.rs | Defines topology_root() returning a SubsystemIdentifier as the single topology identity root. |
| lib/saluki-core/src/topology/interconnect/consumer.rs | Updates tests to build ComponentContext with an explicit topology name. |
| lib/saluki-core/src/topology/ids.rs | Threads topology name into TypedComponentId → ComponentContext creation. |
| lib/saluki-core/src/topology/context.rs | Stores topology name in TopologyContext for consistent context construction. |
| lib/saluki-core/src/topology/component_worker.rs | Removes explicit per-worker resource tracking, relying on supervisor/process naming. |
| lib/saluki-core/src/topology/built.rs | Aligns supervisor names + health registration with canonical identity; passes topology name into interconnect builder. |
| lib/saluki-core/src/topology/blueprint.rs | Aligns resource-accounting registry structure with canonical identity; adds invariant test. |
| lib/saluki-core/src/support.rs | Adds SubsystemIdentifier type for sanitized, hierarchical identifiers. |
| lib/saluki-core/src/runtime/process.rs | Preserves dotted scoping in Name sanitization; changes name storage to MetaString. |
| lib/saluki-core/src/runtime/mod.rs | Re-exports get_sanitized_name (and Name for tests). |
| lib/saluki-core/src/lib.rs | Exposes new support module. |
| lib/saluki-core/src/components/mod.rs | Extends ComponentContext to include topology root and expose canonical identity(). |
| lib/saluki-components/src/transforms/dogstatsd_mapper/mod.rs | Updates tests for new ComponentContext constructors. |
| lib/saluki-components/src/transforms/aggregate/mod.rs | Updates tests for new ComponentContext constructors. |
| lib/saluki-components/src/sources/dogstatsd/mod.rs | Updates tests for new ComponentContext constructors. |
| lib/saluki-components/src/sources/dogstatsd/metrics.rs | Updates tests for new ComponentContext constructors. |
| lib/saluki-components/src/common/otlp/mod.rs | Updates tests for new ComponentContext constructors. |
| lib/saluki-components/src/common/datadog/io.rs | Updates tests for new ComponentContext constructors. |
| lib/resource-accounting/src/registry.rs | Adds ComponentRegistry::full_name() to support asserting canonical naming. |
| bin/agent-data-plane/src/components/ottl_transform_processor/mod.rs | Updates tests for new ComponentContext constructors. |
| bin/agent-data-plane/src/components/ottl_filter_processor/mod.rs | Updates tests for new ComponentContext constructors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Sanitizes a (possibly dotted) name into a dotted string of process-safe segments. | ||
| /// | ||
| /// Periods are treated as segment separators: the input is split on `.`, each segment is sanitized via | ||
| /// [`get_sanitized_name`], and the results are rejoined with `.`. Segments that are empty (or sanitize to empty) are | ||
| /// dropped. Returns `None` if nothing remains. | ||
| fn sanitize_scoped_name(name: &str) -> Option<MetaString> { | ||
| let mut rendered = String::new(); | ||
| for segment in name.split('.') { | ||
| let sanitized = get_sanitized_name(segment); | ||
| if sanitized.is_empty() { | ||
| continue; | ||
| } | ||
|
|
||
| if !rendered.is_empty() { | ||
| rendered.push('.'); | ||
| } | ||
| rendered.push_str(&sanitized); | ||
| } | ||
|
|
||
| if rendered.is_empty() { | ||
| None | ||
| } else { | ||
| Some(rendered.into()) | ||
| } | ||
| } |
| pub fn from_segments<I, S>(segments: I) -> Self | ||
| where | ||
| I: IntoIterator<Item = S>, | ||
| S: AsRef<str>, | ||
| { | ||
| Self { | ||
| segments: segments.into_iter().map(|s| get_sanitized_name(s.as_ref())).collect(), | ||
| } | ||
| } | ||
|
|
||
| /// Consumes the identifier and returns a new one with the given segment appended. | ||
| /// | ||
| /// The segment is sanitized/normalized first. | ||
| pub fn child<S: AsRef<str>>(mut self, segment: S) -> Self { | ||
| self.segments.push(get_sanitized_name(segment.as_ref())); | ||
| self | ||
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ee39ae4fd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.topology_root | ||
| .clone() | ||
| .child(self.component_type.as_category()) | ||
| .child(&*self.component_id) |
There was a problem hiding this comment.
Because ComponentId accepts both hyphens and underscores, routing the raw ID through SubsystemIdentifier::child makes distinct valid same-type components such as foo-bar and foo_bar both render as foo_bar. In a topology that contains both IDs, the second component collides in health registration (and shares the same resource/process identity), so startup fails or accounting merges unrelated components. Consider rejecting IDs that collide after normalization or encoding the raw ID losslessly instead of sanitizing it in-place.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
3ba932e to
6f97b88
Compare
…rmalization/sanitization
2096156 to
d4129ca
Compare
|
Similar to the codex comment I think two different topology names can turn into the same internal ID,
Had codex make a quick test to check for this |
@aqian01 Mmmm, yes. That's suboptimal, although luckily not an issue right now since we never create more than one topology at a time. I'll approach this as follow-up PR since we'll need to make |
Summary
This PR attempts to lay the groundwork to unify how we define the unique identifier for a given component/subsystem within the entire process to drive towards avoiding mismatched identifiers for the same component between different registries.
In Saluki, we have a number of "components" -- topology components, yes, but also more "generic" components like standalone subsystems... think the environment provider, specific background tasks, and so on -- that all end up using some sort of unique name within the process to identify themselves for the purpose of health checking or resource accounting and so on. Ideally, these identifiers would always be the same across every possible registry that wants such an identifier for a single component... but currently that's not the case. This is a problem because it makes it hard to aggregate things across registries: imagine trying to get the live usage of a component (resource accounting) while figuring out how much of its memory bounds that usage represents. Without a stable/consistent identifier between both registries, it becomes hard to do and, ultimately, ends up becoming very fragile.
This PR introduces the concept of
SubsystemIdentifier(don't loveeee the name, might change it) which is meant to represent this shared identifier that a component uses across various registries. It's designed to be built in a hierarchical fashion, so that we define base prefixes that can then be used consistently to build the version of the identifier for each child component and so on.With this, every "component" -- a specific metadata collector in the environment provider, the Aggregate transform in the metrics portion of the topology, etc -- gets a unique name/identifier that is used everywhere: memory bounds, resource accounting, health registry, supervisor worker names, and so on. Most importantly, we accomplish this through a specialized type that ensures the name/identifier is sanitized and normalized, and acts as a way to gate the usages of this name/identifier by requiring the new type at the API boundary instead of any old plain string
This PR doesn't move everything over to using
SubsystemIdentifierjust yet, only the topology components. The rest will happen in a follow-up PR. However, we did a large amount of simplification related toComponentContext: as it now carries the identifier from the topology, it's somewhat easier to generate it once and use it in places that previously usedComponentId, which was the case inTopologyBlueprint,BuiltTopology, andComponentInterconnects, which have now all been simplified to taken advantage of these changes. LikeSubsystemIdentifier, I have more changes planned in this area in the future.Change Type
How did you test this PR?
References
DADP-2