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(global-filters): Introduce global generic filters #3161

Merged
merged 20 commits into from
Mar 1, 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 @@ -8,6 +8,7 @@
- Parametrize transaction in dynamic sampling context. ([#3141](https://github.com/getsentry/relay/pull/3141))
- Adds ReplayVideo envelope-item type. ([#3105](https://github.com/getsentry/relay/pull/3105))
- Parse & scrub span description for supabase. ([#3153](https://github.com/getsentry/relay/pull/3153), [#3156](https://github.com/getsentry/relay/pull/3156))
- Introduce generic filters in global configs. ([#3161](https://github.com/getsentry/relay/pull/3161))

**Bug Fixes**:

Expand Down
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ futures = { version = "0.3", default-features = false, features = ["std"] }
insta = { version = "1.31.0", features = ["json", "redactions", "ron"] }
hash32 = "0.3.1"
hashbrown = "0.14.3"
indexmap = "2.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

We could bump the dependency to 2.2.3 (latest)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to do that in a follow-up if the new version introduces some nice features. I don't want to increase the scope of this PR as it's already big and complex enough.

Copy link
Member

Choose a reason for hiding this comment

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

How would using the latest version increase the scope of this PR? It's a new dependency anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2.0.0 -> 2.2.3 should have no breaking changes, but there could be e.g. uncaught panics. If this is the case and we identify this PR as the root cause, narrowing the cause further to the dependency is more complicated. Separating it in a different PR is easier to spot, and I don't mind the extra bit of work.

itertools = "0.10.5"
once_cell = "1.13.1"
parking_lot = "0.12.1"
Expand Down
3 changes: 2 additions & 1 deletion relay-dynamic-config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ relay-event-normalization = { path = "../relay-event-normalization" }
relay-filter = { path = "../relay-filter" }
relay-log = { path = "../relay-log" }
relay-pii = { path = "../relay-pii" }
relay-protocol = { path = "../relay-protocol" }
relay-protocol = { path = "../relay-protocol" }
relay-quotas = { path = "../relay-quotas" }
relay-sampling = { path = "../relay-sampling" }
serde = { workspace = true }
Expand All @@ -33,3 +33,4 @@ smallvec = { workspace = true }
[dev-dependencies]
insta = { workspace = true }
similar-asserts = { workspace = true }
indexmap = { workspace = true }
38 changes: 38 additions & 0 deletions relay-dynamic-config/src/global.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ use std::path::Path;

use relay_base_schema::metrics::MetricNamespace;
use relay_event_normalization::MeasurementsConfig;
use relay_filter::GenericFiltersConfig;
use relay_quotas::Quota;
use serde::{Deserialize, Serialize};
use serde_json::Value;

use crate::ErrorBoundary;

/// A dynamic configuration for all Relays passed down from Sentry.
///
/// Values shared across all projects may also be included here, to keep
Expand All @@ -23,6 +26,12 @@ pub struct GlobalConfig {
/// Quotas that apply to all projects.
#[serde(skip_serializing_if = "Vec::is_empty")]
pub quotas: Vec<Quota>,
/// Configuration for global inbound filters.
iker-barriocanal marked this conversation as resolved.
Show resolved Hide resolved
///
/// These filters are merged with generic filters in project configs before
/// applying.
#[serde(skip_serializing_if = "skip_generic_filters")]
pub filters: ErrorBoundary<GenericFiltersConfig>,
iker-barriocanal marked this conversation as resolved.
Show resolved Hide resolved
iker-barriocanal marked this conversation as resolved.
Show resolved Hide resolved
/// Sentry options passed down to Relay.
#[serde(
deserialize_with = "default_on_error",
Expand All @@ -46,6 +55,21 @@ impl GlobalConfig {
Ok(None)
}
}

/// Returns the generic inbound filters.
pub fn filters(&self) -> Option<&GenericFiltersConfig> {
match &self.filters {
ErrorBoundary::Err(_) => None,
Copy link
Member

Choose a reason for hiding this comment

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

In this case, we filter nothing, right?

Not a strong opinion on this, because we can always bump the metrics extraction version, but Isn't the following scenario still relevant:

  1. We make a breaking change to the filter config.
  2. Outdated external relays stop filtering, and extract metrics from everything that was previously filtered out.

I'm also on board with just deciding that we'll never make a breaking change, but in that case, why do we need the error boundary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be addressed in bfb8657.

ErrorBoundary::Ok(f) => Some(f),
}
}
}

fn skip_generic_filters(filters_config: &ErrorBoundary<GenericFiltersConfig>) -> bool {
iker-barriocanal marked this conversation as resolved.
Show resolved Hide resolved
match filters_config {
ErrorBoundary::Err(_) => true,
ErrorBoundary::Ok(config) => config.version == 0 && config.filters.is_empty(),
}
}

/// All options passed down from Sentry to Relay.
Expand Down Expand Up @@ -246,6 +270,20 @@ mod tests {
"namespace": null
}
],
"filters": {
"version": 1,
"filters": [
{
"id": "myError",
"isEnabled": true,
"condition": {
"op": "eq",
"name": "event.exceptions",
"value": "myError"
}
}
]
},
"options": {
"profiling.profile_metrics.unsampled_profiles.enabled": true
}
Expand Down
12 changes: 6 additions & 6 deletions relay-dynamic-config/src/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use relay_event_normalization::{
BreakdownsConfig, MeasurementsConfig, PerformanceScoreConfig, SpanDescriptionRule,
TransactionNameRule,
};
use relay_filter::FiltersConfig;
use relay_filter::ProjectFiltersConfig;
use relay_pii::{DataScrubbingConfig, PiiConfig};
use relay_quotas::Quota;
use relay_sampling::SamplingConfig;
Expand Down Expand Up @@ -32,8 +32,8 @@ pub struct ProjectConfig {
#[serde(skip_serializing_if = "Option::is_none")]
pub grouping_config: Option<Value>,
/// Configuration for filter rules.
#[serde(skip_serializing_if = "FiltersConfig::is_empty")]
pub filter_settings: FiltersConfig,
#[serde(skip_serializing_if = "ProjectFiltersConfig::is_empty")]
pub filter_settings: ProjectFiltersConfig,
/// Configuration for data scrubbers.
#[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")]
pub datascrubbing_settings: DataScrubbingConfig,
Expand Down Expand Up @@ -109,7 +109,7 @@ impl Default for ProjectConfig {
trusted_relays: vec![],
pii_config: None,
grouping_config: None,
filter_settings: FiltersConfig::default(),
filter_settings: ProjectFiltersConfig::default(),
datascrubbing_settings: DataScrubbingConfig::default(),
event_retention: None,
quotas: Vec::new(),
Expand Down Expand Up @@ -154,8 +154,8 @@ pub struct LimitedProjectConfig {
pub allowed_domains: Vec<String>,
pub trusted_relays: Vec<PublicKey>,
pub pii_config: Option<PiiConfig>,
#[serde(skip_serializing_if = "FiltersConfig::is_empty")]
pub filter_settings: FiltersConfig,
#[serde(skip_serializing_if = "ProjectFiltersConfig::is_empty")]
pub filter_settings: ProjectFiltersConfig,
#[serde(skip_serializing_if = "DataScrubbingConfig::is_disabled")]
pub datascrubbing_settings: DataScrubbingConfig,
#[serde(skip_serializing_if = "Option::is_none")]
Expand Down
1 change: 1 addition & 0 deletions relay-filter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ publish = false
[dependencies]
ipnetwork = "0.20.0"
once_cell = { workspace = true }
indexmap = { workspace = true }
regex = { workspace = true }
relay-common = { path = "../relay-common" }
relay-event-schema = { path = "../relay-event-schema" }
Expand Down
Loading
Loading