Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(server): Denylist for metrics #2954

Merged
merged 24 commits into from
Jan 22, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Proactively move on-disk spool to memory. ([#2949](https://github.com/getsentry/relay/pull/2949))
- Default missing `Event.platform` and `Event.level` fields during light normalization. ([#2961](https://github.com/getsentry/relay/pull/2961))
- Add possiblity to block metrics with glob-patterns. ([#2954](https://github.com/getsentry/relay/pull/2954))

**Bug Fixes**:

Expand Down
16 changes: 16 additions & 0 deletions relay-dynamic-config/src/metrics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,27 @@ use std::collections::BTreeSet;

use relay_base_schema::data_category::DataCategory;
use relay_common::glob2::LazyGlob;
use relay_common::glob3::GlobPatterns;
use relay_protocol::RuleCondition;
use serde::{Deserialize, Serialize};

use crate::project::ProjectConfig;

/// Configuration for metrics filtering.
#[derive(Debug, Default, Clone, Serialize, Deserialize)]
#[serde(transparent)]
TBS1996 marked this conversation as resolved.
Show resolved Hide resolved
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved
pub struct Metrics {
Dav1dde marked this conversation as resolved.
Show resolved Hide resolved
/// Patterns of names of metrics that we want to filter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Patterns of names of metrics that we want to filter.
/// The list of patterns which used to filter names of the received metrics.

Maybe something like this will sound better?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, isnt it a bit misleading, like we want to say we filter metrics based on their name, not that we filter their names?

how about /// List of patterns for blocking metrics based on their name. ?

pub denied_names: GlobPatterns,
}

impl Metrics {
/// Returns `true` if it contains any names patterns to filter metric names.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is outdated, is_empty does more now

pub fn is_empty(&self) -> bool {
self.denied_names.is_empty()
}
}

/// Rule defining when a target tag should be set on a metric.
#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
Expand Down
14 changes: 13 additions & 1 deletion relay-dynamic-config/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ use crate::defaults;
use crate::error_boundary::ErrorBoundary;
use crate::feature::FeatureSet;
use crate::metrics::{
self, MetricExtractionConfig, SessionMetricsConfig, TaggingRule, TransactionMetricsConfig,
self, MetricExtractionConfig, Metrics, SessionMetricsConfig, TaggingRule,
TransactionMetricsConfig,
};

/// Dynamic, per-DSN configuration passed down from Sentry.
Expand Down Expand Up @@ -88,6 +89,9 @@ pub struct ProjectConfig {
/// relays that might still need them.
#[serde(skip_serializing_if = "Option::is_none")]
pub span_description_rules: Option<Vec<SpanDescriptionRule>>,
/// Configuration for filtering metrics.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Configuration for filtering metrics.
/// Configuration for metrics.

#[serde(default, skip_serializing_if = "skip_deny_metrics")]
TBS1996 marked this conversation as resolved.
Show resolved Hide resolved
pub metrics: ErrorBoundary<Metrics>,
}

impl ProjectConfig {
Expand Down Expand Up @@ -128,6 +132,7 @@ impl Default for ProjectConfig {
tx_name_rules: Vec::new(),
tx_name_ready: false,
span_description_rules: None,
metrics: Default::default(),
}
}
}
Expand All @@ -139,6 +144,13 @@ fn skip_metrics_extraction(boundary: &ErrorBoundary<MetricExtractionConfig>) ->
}
}

fn skip_deny_metrics(boundary: &ErrorBoundary<Metrics>) -> bool {
TBS1996 marked this conversation as resolved.
Show resolved Hide resolved
match boundary {
ErrorBoundary::Err(_) => true,
ErrorBoundary::Ok(metrics) => metrics.is_empty(),
}
}

/// Subset of [`ProjectConfig`] that is passed to external Relays.
///
/// For documentation of the fields, see [`ProjectConfig`].
Expand Down
154 changes: 153 additions & 1 deletion relay-server/src/services/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::time::Duration;
use chrono::{DateTime, Utc};
use relay_base_schema::project::{ProjectId, ProjectKey};
use relay_config::Config;
use relay_dynamic_config::{ErrorBoundary, Feature, LimitedProjectConfig, ProjectConfig};
use relay_dynamic_config::{ErrorBoundary, Feature, LimitedProjectConfig, Metrics, ProjectConfig};
use relay_filter::matches_any_origin;
use relay_metrics::aggregator::AggregatorConfig;
use relay_metrics::{
Expand Down Expand Up @@ -585,6 +585,26 @@ impl Project {
return;
};

Self::apply_metrics_deny_list(&state.config.metrics, metrics);
Self::filter_disabled_namespace(state, metrics);
}

fn apply_metrics_deny_list(deny_list: &ErrorBoundary<Metrics>, buckets: &mut Vec<Bucket>) {
jan-auer marked this conversation as resolved.
Show resolved Hide resolved
let ErrorBoundary::Ok(metrics) = deny_list else {
return;
};

buckets.retain(|bucket| {
if metrics.denied_names.is_match(&bucket.name) {
relay_log::trace!(mri = bucket.name, "dropping metrics due to block list");
false
} else {
true
}
});
}

fn filter_disabled_namespace(state: &ProjectState, metrics: &mut Vec<Bucket>) {
metrics.retain(|metric| {
let Ok(mri) = MetricResourceIdentifier::parse(&metric.name) else {
relay_log::trace!(mri = metric.name, "dropping metrics with invalid MRI");
Expand Down Expand Up @@ -1420,4 +1440,136 @@ mod tests {

assert!(metrics.is_empty());
}

fn get_test_buckets(names: &[&str]) -> Vec<Bucket> {
let create_bucket = |name: &&str| -> Bucket {
let json = json!({
"timestamp": 1615889440,
"width": 10,
"name": name,
"type": "c",
"value": 4.0,
"tags": {
"route": "user_index"
}});

serde_json::from_value(json).unwrap()
};

names.iter().map(create_bucket).collect()
}

fn get_test_bucket_names() -> Vec<&'static str> {
[
"g:transactions/foo@none",
"c:custom/foo@none",
"transactions/foo@second",
"transactions/foo",
"c:custom/foo_bar@none",
"endpoint.response_time",
"endpoint.hits",
"endpoint.parallel_requests",
"endpoint.users",
]
.into()
}

fn apply_pattern_to_names(names: &[&str], patterns: &[&str]) -> Vec<String> {
let mut buckets = get_test_buckets(names);
let deny_list: Metrics = {
let vector_as_string: String = serde_json::to_string(patterns).unwrap();
serde_json::from_str(&vector_as_string).unwrap()
};

Project::apply_metrics_deny_list(&ErrorBoundary::Ok(deny_list), &mut buckets);
buckets.into_iter().map(|bucket| bucket.name).collect()
}

#[test]
fn test_metric_deny_list_exact() {
let names = get_test_bucket_names();
let input_qty = names.len();
let remaining_names = apply_pattern_to_names(&names, &["endpoint.parallel_requests"]);

// There's 1 bucket with that exact name.
let buckets_to_remove = 1;

assert_eq!(remaining_names.len(), input_qty - buckets_to_remove);
}

#[test]
fn test_metric_deny_list_end_glob() {
let names = get_test_bucket_names();
let input_qty = names.len();
let remaining_names = apply_pattern_to_names(&names, &["*foo"]);

// There's 1 bucket name with 'foo' in the end.
let buckets_to_remove = 1;

assert_eq!(remaining_names.len(), input_qty - buckets_to_remove);
}

#[test]
fn test_metric_deny_list_middle_glob() {
let names = get_test_bucket_names();
let input_qty = names.len();
let remaining_names = apply_pattern_to_names(&names, &["*foo*"]);

// There's 4 bucket names with 'foo' in the middle, and one with foo in the end.
let buckets_to_remove = 5;

assert_eq!(remaining_names.len(), input_qty - buckets_to_remove);
}

#[test]
fn test_metric_deny_list_beginning_glob() {
let names = get_test_bucket_names();
let input_qty = names.len();
let remaining_names = apply_pattern_to_names(&names, &["endpoint*"]);

// There's 4 buckets starting with "endpoint".
let buckets_to_remove = 4;

assert_eq!(remaining_names.len(), input_qty - buckets_to_remove);
}

#[test]
fn test_metric_deny_list_everything() {
let names = get_test_bucket_names();
let remaining_names = apply_pattern_to_names(&names, &["*"]);

assert_eq!(remaining_names.len(), 0);
}

#[test]
fn test_metric_deny_list_multiple() {
let names = get_test_bucket_names();
let input_qty = names.len();
let remaining_names = apply_pattern_to_names(&names, &["endpoint*", "*transactions*"]);

let endpoint_buckets = 4;
let transaction_buckets = 3;

assert_eq!(
remaining_names.len(),
input_qty - endpoint_buckets - transaction_buckets
);
}

#[test]
fn test_serialize_metrics_config() {
let input_patterns = vec!["foo".to_string(), "bar*".to_string()];

let deny_list: Metrics = {
let vector_as_string: String = serde_json::to_string(&input_patterns).unwrap();
serde_json::from_str(&vector_as_string).unwrap()
};

let back_to_vec: Vec<String> = {
let string_rep = serde_json::to_string(&deny_list.denied_names).unwrap();
serde_json::from_str(&string_rep).unwrap()
};

assert_eq!(input_patterns, back_to_vec);
}
}
Loading