feat(metrics): Block metric tags with glob pattern #2973
Conversation
There was a problem hiding this comment.
Could you add an integration test that shows this new config being used? It would be great to get some input from @obostjancic on the usability of this config.
relay-dynamic-config/src/metrics.rs
Outdated
| @@ -21,15 +21,29 @@ pub struct Metrics { | |||
| /// List of patterns for blocking metrics based on their name. | |||
| #[serde(skip_serializing_if = "GlobPatterns::is_empty")] | |||
| pub denied_names: GlobPatterns, | |||
| /// List of patterns of tags to remove. | |||
There was a problem hiding this comment.
Could you add a bit more documentation on what that does? In contrast to the option above, it is worth pointing out that removing tags doesn't drop the overall metric bucket (i.e. the metric is still queryable without the removed tags).
You can also expand the docs on TagBlock, in which case it would be useful to actively refer to the docs on the struct.
relay-server/src/services/project.rs
Outdated
| relay_log::trace!(mri = bucket.name, "dropping metrics due to block list"); | ||
| false | ||
| } else if Self::metric_namespace_invalid(state, bucket) { |
There was a problem hiding this comment.
metric_namespace_invalid tracks trace logs internally when it drops the bucket, but metric_name_denied does not. It's easy to miss this difference, so could we align this?
This function actually drops due to two reasons: an MRI that cannot be parsed, or an unknown namespace. If you prefer to have this in sub functions, I'd suggest to split those into two functions that just return booleans, and then log the messages here at the upper level. This would require you to pass the parsed MRI into the namespace function instead of the entire bucket.
There was a problem hiding this comment.
agree thats better! I moved it up to the upper function now.
relay-server/src/services/project.rs
Outdated
| buckets.retain(|bucket| { | ||
| if metrics.denied_names.is_match(&bucket.name) { | ||
| metrics.retain_mut(|bucket| { | ||
| if Self::metric_name_denied(metrics_config, bucket) { |
There was a problem hiding this comment.
All of these functions are not associated with the Project struct in any way. Can we make them free functions?
cc @iambriccardo since he is implementing blocking endpoint in getsentry/sentry#63598 |
Done! |
| #[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq)] | ||
| pub struct TagBlock { | ||
| /// Name of metric of which we want to remove certain tags. | ||
| pub name: GlobPatterns, |
There was a problem hiding this comment.
cc @obostjancic : If the product now wants to drop a tag for all metrics in the custom namespace, you would configure the glob pattern "?:custom/*".
#2797
This PR introduces a way of deleting certain tags based on glob pattern matching.