-
Notifications
You must be signed in to change notification settings - Fork 91
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
Changes from 12 commits
6bd0907
bba2a76
117bc03
950325d
45e2f94
b57c122
6fafec0
8384897
8aee088
a6db146
9d3c3b7
d7790ea
791ac3a
bfb8657
67014c0
e47c900
eb784c7
7454814
28ed597
93dfac9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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", | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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 | ||
} | ||
|
There was a problem hiding this comment.
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)There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.