feat(dogstatsd): allowlist tag values by metric prefix - #2271
feat(dogstatsd): allowlist tag values by metric prefix#2271lukesteensen wants to merge 1 commit into
Conversation
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The allowlist implementation and typed configuration path cover the reviewed prefix, replacement, bare/empty tag, origin-tag, overlap, cache, and mapper/namespace scenarios without a clear diff-only regression. Focused Rust execution was unavailable because the sandbox could not fetch the pinned hyper-http-proxy git dependency; no additional tests recommended: existing adversarial unit coverage already exercises the relevant branches.
🤖 Datadog Autotest · Commit a63f659 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
![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: a63f659d92
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a63f659 to
f92621d
Compare
Binary Size Analysis (Agent Data Plane)Baseline: 0f47357 · Comparison: 257b400 · 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 ( |
There was a problem hiding this comment.
This looks great over all, except for one sticking point which is what should happen if bad config reaches the component (currently a panic).
Also I have a question. Is this feature development that is mirroring an Agent behavior? Or is this "green field" feature development on the ADP-side only?
This is an important question for testing because currently we would use a correctness test for integration to see that we match Agent behavior. But we don't have a great mechanism for testing greenfield behavior at the moment.
| ChainedConfiguration::default().with_transform_builder("dogstatsd_mapper", dsd_mapper_config); | ||
| let dsd_tag_filterlist_config = TagFilterlistConfiguration::from_configuration(config) | ||
| let dogstatsd_config = config_system.live(|config| &config.domains.dogstatsd); | ||
| let dsd_tag_filterlist_config = TagFilterlistConfiguration::from_configuration(dogstatsd_config) |
There was a problem hiding this comment.
Good change. Switching from GenericConfiguration to Live<dogstatsd::Domain>.
| "metric_tag_filterlist": [{ | ||
| "metric_name": "tenant.mapped.requests", | ||
| "action": "exclude", | ||
| "tags": ["remove"] | ||
| }], |
There was a problem hiding this comment.
Makes sense because Metric Tag Filter list switched to typed config. 👍
| self.filters = compile_all_filters(&new_entries, &self.value_allowlists) | ||
| .expect("the static value allow-list was validated when the transform was built"); |
There was a problem hiding this comment.
This makes me a little bit nervous. If the validation at the config level is sufficient, then ideally the compile_all_filters function would not need to return a Result. But if it legitimately can fail because the config layer cannot be expected to sufficiently validate incoming filters, then something better than panicking probably needs to be done.
I think the problem to resolve is this:
- If the config layer can't completely validate the incoming filter list, and
- later, the component determines the list to be invalid...
There isn't really a graceful way out. You have accepted configuration that the component cannot express. If that's really the case then perhaps a panic is the right thing to do. But a better solution would be to try and make sure a bad config cannot be accepted by the system.
Note that, the way the config system is set up right now, on system startup, bad config fails the system. Later once we are serving, a bad partial config update (i.e. a remote config change) will report an error but we will continue running on the last good config.
There was a problem hiding this comment.
This makes me nervous because I don't actually think it's correct?
If we assume that we validated the value allowlist at first by virtue of making it to this point, there's still the logic in add_value_allowlists that checks for metric prefix overlap, which might occur at runtime if the new incoming metric tag filter configuration changes significantly?
Maybe I'm misreading it, but that looks sus to me... and it's not immediately clear how to reconcile that.
There was a problem hiding this comment.
Yeah, tried to rework this to be clearer. I won't claim my understanding of the config system is 100% yet, but we should now be fully parsing and validating in the config system itself. Runtime updates should also end up replacing the full key after validation, so partial updates can't introduce an overlap.
Regression Detector (Agent Data Plane, full suite)Optimization Goals:
|
|
|
||
| use self::telemetry::Telemetry; | ||
| struct CompiledTagValueAllowlist { | ||
| allowed_values: HashSet<String, FoldHashState>, |
There was a problem hiding this comment.
Can we just import FastHashSet and use the alias directly here? I realize we're explicitly importing hashbrown versions here, vs the recommended aliases pointing at stdlib versions, but it doesn't seem like we actually use anything from hashbrown that we can't get from stdlib, so...
(Same with HashMap further down.)
| /// - Same metric name + conflicting actions → `exclude` wins. | ||
| pub fn compile_filters(entries: &[MetricTagFilterEntry]) -> CompiledFilters { | ||
| let mut filters: CompiledFilters = HashMap::with_hasher(FoldHashState::default()); | ||
| let mut filters = CompiledFilters { |
There was a problem hiding this comment.
We should just be able to derive Default on CompiledFilters and then use CompiledFilters::default() here.
| /// # Errors | ||
| /// | ||
| /// Returns an error when two entries for the same tag have overlapping metric prefixes. | ||
| pub fn add_value_allowlists( |
There was a problem hiding this comment.
A general comment on the logic here: can we just do all of the string trimming automatically on behalf of the user?
There was a problem hiding this comment.
I revisited this and got rid of the trimming to be consistent with what we do for the full tag feature. Seemed a little overly aggressive on second thought.
| self.filters = compile_all_filters(&new_entries, &self.value_allowlists) | ||
| .expect("the static value allow-list was validated when the transform was built"); |
There was a problem hiding this comment.
This makes me nervous because I don't actually think it's correct?
If we assume that we validated the value allowlist at first by virtue of making it to this point, there's still the logic in add_value_allowlists that checks for metric prefix overlap, which might occur at runtime if the new incoming metric tag filter configuration changes significantly?
Maybe I'm misreading it, but that looks sus to me... and it's not immediately clear how to reconcile that.
f92621d to
2b7632e
Compare
2b7632e to
257b400
Compare
3 participants
Summary
Add
metric_tag_value_allowlist, which limits DogStatsD metric cardinality by retaining selected values for a tag and removing or replacing values outside the allow-list.Rules select metrics using case-sensitive metric-name prefixes:
This change also moves the existing
metric_tag_filterlistcomponent from component-local deserialization andGenericConfigurationaccess to the typed DogStatsD configuration domain. Dynamic whole-tag updates continue through a narrowLive<T>view, while value allow-list rules remain static.Design decisions
statsd_metric_namespaceprefixing, so rules describe names as aggregation sees them.includerule must retain the value-filtered tag name or the whole-tag rule removes it before value filtering.customer_idhas no value and remains unchanged.customer_id:has the value""and is subject to the allow-list.metric_tag_value_allowlistrequires an ADP restart. Remote Config updates tometric_tag_filterlistdo not replace value rules.:in tag names, overlapping same-tag prefixes, and surrounding whitespace. Empty replacements and replacement collisions remain operator policy choices.Change Type
How did you test this PR?
make fmtcargo check --workspacecargo check --workspace --testsmake test-alldsd-tag-filterlistcorrectness casegit diff --checkmake check-allpasses formatting and full-workspace Clippy, then stops on an existing Vale spelling error in unchangeddocs/development/contributing.md(PR's). The remaining check targets were run separately and passed.References
N/A