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: 92e9dc0 · Comparison: f92621d · 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. 👍
| } | ||
|
|
||
| fn deserialize_newtype_struct<V>(self, _name: &'static str, visitor: V) -> Result<V::Value, Self::Error> | ||
| fn deserialize_newtype_struct<V>(mut self, name: &'static str, visitor: V) -> Result<V::Value, Self::Error> |
There was a problem hiding this comment.
I don't understand this change but trust that the coding agent does! (For context this is how we derive environment variable keys from nested struct keys).
| use serde::{Deserialize, Deserializer}; | ||
|
|
||
| // The environment path recorder recognizes this serde newtype marker and decodes the leaf as JSON. | ||
| pub(crate) const JSON_SEQUENCE_MARKER: &str = "SalukiJsonSequence"; |
There was a problem hiding this comment.
Oh, I see we are encoding JSON as a value in the environment variable and need a way to recognize that during deserialization.
| 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.
2 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