-
Notifications
You must be signed in to change notification settings - Fork 93
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(profiling): Support inbound filters for profiles #4176
feat(profiling): Support inbound filters for profiles #4176
Conversation
This adds support for inbound filters on profile types specifically allowing to filter profiles based on releases.
…upport-inbound-filters-for-profiles
…upport-inbound-filters-for-profiles
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.
@Zylphrex could you add an integration test for this? Something along the lines of
relay/tests/integration/test_filters.py
Lines 287 to 309 in 5b0798d
def test_client_ip_filters_are_applied( | |
mini_sentry, | |
relay, | |
): | |
""" | |
Test that relay normalizes messages when processing is enabled and sends them via Kafka queues | |
""" | |
relay = relay(mini_sentry) | |
project_id = 42 | |
project_config = mini_sentry.add_full_project_config(project_id) | |
filter_settings = project_config["config"]["filterSettings"] | |
filter_settings["clientIps"] = {"blacklistedIps": ["1.2.3.0/24"]} | |
event = {"message": "foo"} | |
relay.send_event(project_id, event, headers={"X-Forwarded-For": "1.2.3.4"}) | |
report = mini_sentry.get_client_report() | |
assert report["filtered_events"] == [ | |
{"reason": "ip-address", "category": "error", "quantity": 1} | |
] | |
assert mini_sentry.captured_events.empty() |
relay
and mini_sentry
should be enough, no need to spawn relay_with_processing
).
Rest of the PR looks good to me!
@@ -75,10 +80,55 @@ struct MinimalProfile { | |||
#[serde(alias = "profile_id", alias = "chunk_id")] | |||
event_id: ProfileId, | |||
platform: String, | |||
release: Option<String>, |
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.
Will we filter on more fields in the future? Users might be surprised when they configure a release filter, see that it works, and then try the same for ip_addr and it fails.
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.
For ip_addr
, I see that it's pulled from the user for other event types. Meaning for a backend event, it'll use the frontend user IP. Is this a strict requirement? Because for continuous profiles, we can only send the ip of the server in some cases.
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.
So far the filter has only been applied to the end-user IP like you said, so in this case I would leave it empty (None
) for continuous profiles.
#[serde(default)] | ||
version: sample::Version, | ||
} | ||
|
||
impl Filterable for MinimalProfile { |
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.
Not directly related to this PR, but it might be worth defaulting these methods in the trait.
This adds support for inbound filters on profile types specifically allowing to filter profiles based on releases.