enhancement(context): allow mutating tagsets on metric contexts#1270
enhancement(context): allow mutating tagsets on metric contexts#1270tobz wants to merge 1 commit intotobz/core-data-model-sharedtagset-to-tagsetfrom
Conversation
dd-octo-sts
bot
added
area/core
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull request overview
Adds copy-on-write tag mutation support to Context so metric transforms can enrich tags without rebuilding contexts, and refactors the host-tags transform to use the new API.
Changes:
- Added
Context::with_tags_mut/Context::with_origin_tags_mutwith COW semantics and automatic key rehashing. - Updated host-tags enrichment to mutate metric contexts in place instead of resolving a new context.
- Consolidated/adjusted TagSet property tests and imports.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| lib/saluki-context/src/tags/tagset/owned.rs | Test module cleanup and minor proptest strategy adjustments/renames. |
| lib/saluki-context/src/context.rs | Introduces mutable tag/origin-tag mutation APIs and adds unit tests for COW + rehash behavior. |
| lib/saluki-components/src/transforms/host_tags/mod.rs | Removes resolver-based context rebuilding and switches to in-place tag mutation for host-tag enrichment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(context) = resolver.resolve_with_origin_tags(metric.context().name(), tags, origin_tags) { | ||
| *metric.context_mut() = context; | ||
| } | ||
| metric.context_mut().with_tags_mut(|tags| tags.merge_shared(host_tags)); |
There was a problem hiding this comment.
merge_shared explicitly does not avoid duplicate tags, so this enrichment can introduce duplicates when a metric already has one of the host tags (or if enrichment is applied more than once). This changes behavior vs the previous implementation (which deduped via collect::<TagSet>()) and can inflate tag lists / affect downstream hashing and serialization. Consider using merge_missing_shared (or otherwise deduping) when merging host tags into the metric context.
| metric.context_mut().with_tags_mut(|tags| tags.merge_shared(host_tags)); | |
| metric | |
| .context_mut() | |
| .with_tags_mut(|tags| tags.merge_missing_shared(host_tags)); |
There was a problem hiding this comment.
The feedback is valid, but in this case, we actually don't care about duplicates:
- host tags are incredibly unlikely to clash with actual instrumented tags
- we still dedupe during encoding to allow for more efficient chaining earlier in the pipeline
| @@ -65,19 +54,8 @@ impl SynchronousTransformBuilder for HostTagsConfiguration { | |||
| .map(Tag::from) | |||
| .collect::<SharedTagSet>(); | |||
|
|
|||
| let context_string_interner_size = | |||
| NonZeroUsize::new(self.host_tags_context_string_interner_bytes.as_u64() as usize) | |||
| .ok_or_else(|| generic_error!("host_tags_context_string_interner_bytes must be greater than 0")) | |||
| .unwrap(); | |||
| let context_resolver = | |||
| ContextResolverBuilder::from_name(format!("{}/host_tags/primary", context.component_id())) | |||
| .expect("resolver name is not empty") | |||
| .with_interner_capacity_bytes(context_string_interner_size) | |||
| .with_idle_context_expiration(Duration::from_secs(30)) | |||
| .build(); | |||
| Ok(Box::new(HostTagsEnrichment { | |||
| start: Instant::now(), | |||
| context_resolver: Some(context_resolver), | |||
| expected_tags_duration: Duration::from_secs(self.expected_tags_duration), | |||
| host_tags: Some(host_tags), | |||
| })) | |||
There was a problem hiding this comment.
This transform used to rebuild contexts via a ContextResolver, which provides interning + caching of resolved contexts. With the new in-place with_tags_mut approach, any metric contexts that are shared (common with ContextResolver-produced contexts) will be detached via Arc::make_mut, potentially producing one enriched ContextInner per metric and losing cross-metric sharing. If host-tag enrichment is expected to touch many metrics, consider retaining a resolver/cache for enriched contexts or another strategy that preserves sharing.
There was a problem hiding this comment.
Host Tags transform generally only operates for a few minutes after startup, so the reduction in sharing due to creating an off-shoot Context, and not caching that via something like ContextResolver, is pretty small and not sustained over time.
However, it could be useful to add something in ContextResolver where we support modifying how the input pieces are used to build the resulting context. In order to actually make use of ContextResolver in this scenario, we'd want to feed it to the incoming metric context untouched, and then generate a new context (which is the one we'd cache) with the additional host tags bolted on... such that we didn't have to do the chaining/additional host tags for subsequent lookups.
We could do a cache, too, but ContextResolver already wraps up most of the things we want here -- cache config, expiration, telemetry, etc -- in a nice package that's specific to contexts and so it'd be nicer to somehow add it there than to just use a cache naïvely.
| /// Mutates the instrumented tags of this context via a closure. | ||
| /// | ||
| /// Uses copy-on-write semantics: if this context shares its inner data with other clones, the | ||
| /// inner data is cloned first so that mutations do not affect other holders. If this context is | ||
| /// the sole owner, the mutation happens in place. | ||
| /// | ||
| /// The context key is automatically recomputed after the closure returns. | ||
| pub fn with_tags_mut(&mut self, f: impl FnOnce(&mut TagSet)) { | ||
| self.mutate_inner(|inner| f(&mut inner.tags)); | ||
| } | ||
|
|
||
| /// Mutates the origin tags of this context via a closure. | ||
| /// | ||
| /// Uses copy-on-write semantics: if this context shares its inner data with other clones, the | ||
| /// inner data is cloned first so that mutations do not affect other holders. If this context is | ||
| /// the sole owner, the mutation happens in place. | ||
| /// | ||
| /// The context key is automatically recomputed after the closure returns. | ||
| pub fn with_origin_tags_mut(&mut self, f: impl FnOnce(&mut TagSet)) { | ||
| self.mutate_inner(|inner| f(&mut inner.origin_tags)); | ||
| } |
There was a problem hiding this comment.
The new with_tags_mut/with_origin_tags_mut methods mutate fields that contribute to Context's Eq/Hash (via the recomputed key). Please document that callers must not mutate a Context while it is being used as a key in a HashMap/HashSet (or otherwise relied on for stable hashing), as this would break those data structures' invariants.
There was a problem hiding this comment.
This is true, except for the fact that normal collections (certainly the above ones from stdlib) don't actually allow modifying the key (or value, if HashSet) after insertion... and we're not doing interior mutability tricks here so we're not subverting that, and thus this isn't actually a problem for us.
| fn mutate_inner(&mut self, f: impl FnOnce(&mut ContextInner)) { | ||
| let inner = Arc::make_mut(&mut self.inner); | ||
| f(inner); | ||
| let (key, _) = hash_context(&inner.name, &inner.tags, &inner.origin_tags); | ||
| inner.key = key; | ||
| } |
There was a problem hiding this comment.
mutate_inner recomputes the context key via hash_context, which allocates a new PrehashedHashSet each call. If with_tags_mut is used in per-metric hot paths (e.g. enrichment), this extra allocation can be a noticeable cost. Consider using the non-allocating hash_context_with_seen with a reusable seen set (or another allocation-free rehash strategy) to avoid per-mutation allocations.
There was a problem hiding this comment.
This is good advice, and we might consider a thread-local seen that can be reused, which would also help us clean up the different versions of the function, the callsites using them which carry around their own PrehashedHashSet, and so on.
Good for a follow-up PR.
Binary Size Analysis (Agent Data Plane)Target: b0ed855 (baseline) vs 88413de (comparison) diff
|
| Module | File Size | Symbols |
|---|---|---|
saluki_context::tags::tagset |
+9.30 KiB | 19 |
saluki_components::transforms::host_tags |
-4.56 KiB | 8 |
core |
+4.13 KiB | 160 |
agent_data_plane::state::metrics |
+3.99 KiB | 3 |
figment |
-3.11 KiB | 17 |
saluki_context::hash::hash_context |
+2.87 KiB | 1 |
axum |
+2.66 KiB | 2 |
saluki_context::hash::hash_context_with_seen |
-1.99 KiB | 1 |
saluki_components::destinations::dsd_stats |
+1.99 KiB | 1 |
saluki_components::sources::otlp |
+1.74 KiB | 5 |
saluki_context::resolver::ContextResolver |
-1.64 KiB | 4 |
saluki_env::workload::metadata |
-1.54 KiB | 1 |
saluki_context::resolver::TagsResolver |
-1.21 KiB | 8 |
agent_data_plane::cli::run |
-1.04 KiB | 1 |
smallvec |
+975 B | 10 |
saluki_env::workload::collectors |
+946 B | 7 |
saluki_env::workload::stores |
-811 B | 2 |
saluki_context::context::Context |
-797 B | 3 |
saluki_components::transforms::apm_stats |
+793 B | 4 |
saluki_env::workload::aggregator |
+765 B | 1 |
Detailed Symbol Changes
FILE SIZE VM SIZE
-------------- --------------
[NEW] +22.7Ki [NEW] +22.6Ki saluki_components::sources::otlp::logs::transform::transform_log_record::h510ca1204d3ade65
[NEW] +18.5Ki [NEW] +18.3Ki _<saluki_components::transforms::host_tags::HostTagsConfiguration as saluki_core::components::transforms::builder::SynchronousTransformBuilder>::build::_{{closure}}::h6032f81f6d4dd9d0
+0.7% +16.9Ki +0.8% +15.0Ki [762 Others]
[NEW] +10.6Ki [NEW] +10.5Ki <saluki_core::data_model::event::Event as core::clone::Clone>::clone.10057
[NEW] +5.52Ki [NEW] +5.38Ki <hickory_proto::rr::record_data::RData as core::clone::Clone>::clone.8514
[NEW] +5.13Ki [NEW] +5.00Ki <hickory_proto::rr::record_data::RData as core::clone::Clone>::clone.11201
[NEW] +4.71Ki [NEW] +4.59Ki <rustls::error::Error as core::clone::Clone>::clone.10035
[NEW] +4.71Ki [NEW] +4.59Ki <rustls::error::Error as core::clone::Clone>::clone.11216
[NEW] +3.87Ki [NEW] +3.76Ki saluki_components::common::otlp::attributes::tags_from_attributes::h510bddeea173bf1d
[NEW] +3.83Ki [NEW] +3.71Ki <webpki::error::Error as core::fmt::Debug>::fmt.11394
[NEW] +3.70Ki [NEW] +3.60Ki saluki_context::resolver::TagsResolver::create_tag_set::h85bfa90ac5da346a
[DEL] -3.83Ki [DEL] -3.71Ki <webpki::error::Error as core::fmt::Debug>::fmt.11397
[DEL] -3.94Ki [DEL] -3.83Ki saluki_components::common::otlp::attributes::tags_from_attributes::h2b53e1f4f1fdf965
[DEL] -4.71Ki [DEL] -4.59Ki <rustls::error::Error as core::clone::Clone>::clone.11219
[DEL] -4.71Ki [DEL] -4.59Ki <rustls::error::Error as core::clone::Clone>::clone.10041
[DEL] -5.13Ki [DEL] -5.00Ki <hickory_proto::rr::record_data::RData as core::clone::Clone>::clone.11204
[DEL] -5.16Ki [DEL] -4.96Ki _<saluki_components::transforms::host_tags::HostTagsEnrichment as saluki_core::components::transforms::SynchronousTransform>::transform_buffer::h8298bc763a5b1ed2
[DEL] -5.52Ki [DEL] -5.38Ki <hickory_proto::rr::record_data::RData as core::clone::Clone>::clone.8520
[DEL] -10.7Ki [DEL] -10.6Ki <saluki_core::data_model::event::Event as core::clone::Clone>::clone.10063
[DEL] -19.3Ki [DEL] -19.1Ki _<saluki_components::transforms::host_tags::HostTagsConfiguration as saluki_core::components::transforms::builder::SynchronousTransformBuilder>::build::_{{closure}}::h51276be7dee73d6d
[DEL] -22.9Ki [DEL] -22.8Ki saluki_components::sources::otlp::logs::transform::transform_log_record::h1731e541914d182a
+0.1% +14.3Ki +0.1% +12.5Ki TOTAL
Regression Detector (Agent Data Plane)Regression Detector ResultsRun ID: 7e4eb572-6543-455d-b630-ec2ff0a4350d Baseline: b0ed855 Optimization Goals: ✅ No significant changes detected
|
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | +2.26 | [-2.70, +7.22] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.02 | [-0.15, +0.10] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | -3.53 | [-3.99, -3.06] | 1 | (metrics) (profiles) (logs) |
Fine details of change detection per experiment
| perf | experiment | goal | Δ mean % | Δ mean % CI | trials | links |
|---|---|---|---|---|---|---|
| ➖ | otlp_ingest_metrics_5mb_cpu | % cpu utilization | +5.82 | [-3.22, +14.86] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_cpu | % cpu utilization | +3.42 | [+1.18, +5.66] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_cpu | % cpu utilization | +2.96 | [-52.33, +58.25] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_cpu | % cpu utilization | +2.26 | [-2.70, +7.22] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_cpu | % cpu utilization | +1.43 | [-0.98, +3.84] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_ultraheavy | memory utilization | +0.98 | [+0.84, +1.11] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_cpu | % cpu utilization | +0.59 | [-5.61, +6.80] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_memory | memory utilization | +0.52 | [+0.33, +0.70] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_memory | memory utilization | +0.39 | [+0.23, +0.56] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_medium | memory utilization | +0.33 | [+0.13, +0.52] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_idle | memory utilization | +0.30 | [+0.26, +0.34] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_heavy | memory utilization | +0.30 | [+0.16, +0.44] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_memory | memory utilization | +0.22 | [+0.04, +0.41] | 1 | (metrics) (profiles) (logs) |
| ➖ | quality_gates_rss_dsd_low | memory utilization | +0.17 | [-0.03, +0.37] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_throughput | ingress throughput | +0.02 | [-0.10, +0.13] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_throughput | ingress throughput | +0.00 | [-0.14, +0.15] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_throughput | ingress throughput | +0.00 | [-0.05, +0.05] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_throughput | ingress throughput | +0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_throughput | ingress throughput | -0.00 | [-0.06, +0.06] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_throughput | ingress throughput | -0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_throughput | ingress throughput | -0.00 | [-0.02, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_100mb_3k_contexts_throughput | ingress throughput | -0.01 | [-0.04, +0.02] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_throughput | ingress throughput | -0.02 | [-0.15, +0.10] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_1mb_3k_contexts_memory | memory utilization | -0.04 | [-0.21, +0.14] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_filtering_5mb_memory | memory utilization | -0.09 | [-0.43, +0.24] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_memory | memory utilization | -0.18 | [-0.35, -0.00] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_cpu | % cpu utilization | -0.27 | [-1.74, +1.20] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_memory | memory utilization | -0.39 | [-0.63, -0.15] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_5mb_cpu | % cpu utilization | -0.42 | [-2.62, +1.77] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_10mb_3k_contexts_cpu | % cpu utilization | -0.52 | [-31.36, +30.32] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_traces_ottl_transform_5mb_memory | memory utilization | -0.73 | [-0.97, -0.48] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_metrics_5mb_memory | memory utilization | -1.09 | [-1.33, -0.84] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_512kb_3k_contexts_cpu | % cpu utilization | -1.64 | [-57.83, +54.55] | 1 | (metrics) (profiles) (logs) |
| ➖ | dsd_uds_500mb_3k_contexts_throughput | ingress throughput | -2.78 | [-2.91, -2.64] | 1 | (metrics) (profiles) (logs) |
| ➖ | otlp_ingest_logs_5mb_memory | memory utilization | -3.53 | [-3.99, -3.06] | 1 | (metrics) (profiles) (logs) |
Bounds Checks: ✅ Passed
| perf | experiment | bounds_check_name | replicates_passed | observed_value | links |
|---|---|---|---|---|---|
| ✅ | quality_gates_rss_dsd_heavy | memory_usage | 10/10 | 114.07MiB ≤ 140MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_low | memory_usage | 10/10 | 33.54MiB ≤ 50MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_medium | memory_usage | 10/10 | 54.13MiB ≤ 75MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_dsd_ultraheavy | memory_usage | 10/10 | 168.78MiB ≤ 200MiB | (metrics) (profiles) (logs) |
| ✅ | quality_gates_rss_idle | memory_usage | 10/10 | 21.03MiB ≤ 40MiB | (metrics) (profiles) (logs) |
Explanation
Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%
Performance changes are noted in the perf column of each table:
- ✅ = significantly better comparison variant performance
- ❌ = significantly worse comparison variant performance
- ➖ = no significant change in performance
A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".
For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:
-
Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
-
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
-
Its configuration does not mark it "erratic".
Summary
Change Type
How did you test this PR?
References