-
Notifications
You must be signed in to change notification settings - Fork 94
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
Changes from all commits
f230001
48ccc55
f56cc46
f35cffb
ea2856e
5b0798d
54097ea
9b499c9
0ca0fc1
d097ec6
696edb4
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 |
---|---|---|
|
@@ -40,10 +40,15 @@ | |
//! Relay will forward those profiles encoded with `msgpack` after unpacking them if needed and push a message on Kafka. | ||
|
||
use std::error::Error; | ||
use std::net::IpAddr; | ||
use std::time::Duration; | ||
use url::Url; | ||
|
||
use relay_base_schema::project::ProjectId; | ||
use relay_event_schema::protocol::{Event, EventId}; | ||
use relay_dynamic_config::GlobalConfig; | ||
use relay_event_schema::protocol::{Csp, Event, EventId, Exception, LogEntry, Values}; | ||
use relay_filter::{Filterable, ProjectFiltersConfig}; | ||
use relay_protocol::{Getter, Val}; | ||
use serde::Deserialize; | ||
use serde_json::Deserializer; | ||
|
||
|
@@ -75,10 +80,55 @@ struct MinimalProfile { | |
#[serde(alias = "profile_id", alias = "chunk_id")] | ||
event_id: ProfileId, | ||
platform: String, | ||
release: Option<String>, | ||
#[serde(default)] | ||
version: sample::Version, | ||
} | ||
|
||
impl Filterable for MinimalProfile { | ||
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. Not directly related to this PR, but it might be worth defaulting these methods in the trait. |
||
fn csp(&self) -> Option<&Csp> { | ||
None | ||
} | ||
|
||
fn exceptions(&self) -> Option<&Values<Exception>> { | ||
None | ||
} | ||
|
||
fn ip_addr(&self) -> Option<&str> { | ||
None | ||
} | ||
|
||
fn logentry(&self) -> Option<&LogEntry> { | ||
None | ||
} | ||
|
||
fn release(&self) -> Option<&str> { | ||
self.release.as_deref() | ||
} | ||
|
||
fn transaction(&self) -> Option<&str> { | ||
None | ||
} | ||
|
||
fn url(&self) -> Option<Url> { | ||
None | ||
} | ||
|
||
fn user_agent(&self) -> Option<&str> { | ||
None | ||
} | ||
} | ||
|
||
impl Getter for MinimalProfile { | ||
fn get_value(&self, path: &str) -> Option<Val<'_>> { | ||
match path.strip_prefix("event.")? { | ||
"release" => self.release.as_deref().map(|release| release.into()), | ||
"platform" => Some(self.platform.as_str().into()), | ||
_ => None, | ||
} | ||
} | ||
} | ||
|
||
fn minimal_profile_from_json( | ||
payload: &[u8], | ||
) -> Result<MinimalProfile, serde_path_to_error::Error<serde_json::Error>> { | ||
|
@@ -139,7 +189,13 @@ pub fn parse_metadata(payload: &[u8], project_id: ProjectId) -> Result<ProfileId | |
Ok(profile.event_id) | ||
} | ||
|
||
pub fn expand_profile(payload: &[u8], event: &Event) -> Result<(ProfileId, Vec<u8>), ProfileError> { | ||
pub fn expand_profile( | ||
payload: &[u8], | ||
event: &Event, | ||
client_ip: Option<IpAddr>, | ||
filter_settings: &ProjectFiltersConfig, | ||
global_config: &GlobalConfig, | ||
) -> Result<(ProfileId, Vec<u8>), ProfileError> { | ||
let profile = match minimal_profile_from_json(payload) { | ||
Ok(profile) => profile, | ||
Err(err) => { | ||
|
@@ -156,6 +212,16 @@ pub fn expand_profile(payload: &[u8], event: &Event) -> Result<(ProfileId, Vec<u | |
return Err(ProfileError::InvalidJson(err)); | ||
} | ||
}; | ||
|
||
if let Err(filter_stat_key) = relay_filter::should_filter( | ||
&profile, | ||
client_ip, | ||
filter_settings, | ||
global_config.filters(), | ||
) { | ||
return Err(ProfileError::Filtered(filter_stat_key)); | ||
} | ||
|
||
let transaction_metadata = extract_transaction_metadata(event); | ||
let transaction_tags = extract_transaction_tags(event); | ||
let processed_payload = match (profile.platform.as_str(), profile.version) { | ||
|
@@ -200,7 +266,12 @@ pub fn expand_profile(payload: &[u8], event: &Event) -> Result<(ProfileId, Vec<u | |
} | ||
} | ||
|
||
pub fn expand_profile_chunk(payload: &[u8]) -> Result<Vec<u8>, ProfileError> { | ||
pub fn expand_profile_chunk( | ||
payload: &[u8], | ||
client_ip: Option<IpAddr>, | ||
filter_settings: &ProjectFiltersConfig, | ||
global_config: &GlobalConfig, | ||
) -> Result<Vec<u8>, ProfileError> { | ||
let profile = match minimal_profile_from_json(payload) { | ||
Ok(profile) => profile, | ||
Err(err) => { | ||
|
@@ -212,6 +283,16 @@ pub fn expand_profile_chunk(payload: &[u8]) -> Result<Vec<u8>, ProfileError> { | |
return Err(ProfileError::InvalidJson(err)); | ||
} | ||
}; | ||
|
||
if let Err(filter_stat_key) = relay_filter::should_filter( | ||
&profile, | ||
client_ip, | ||
filter_settings, | ||
global_config.filters(), | ||
) { | ||
return Err(ProfileError::Filtered(filter_stat_key)); | ||
} | ||
|
||
match (profile.platform.as_str(), profile.version) { | ||
("android", _) => android::chunk::parse(payload), | ||
(_, sample::Version::V2) => { | ||
|
@@ -246,18 +327,39 @@ mod tests { | |
#[test] | ||
fn test_expand_profile_with_version() { | ||
let payload = include_bytes!("../tests/fixtures/sample/v1/valid.json"); | ||
assert!(expand_profile(payload, &Event::default()).is_ok()); | ||
assert!(expand_profile( | ||
payload, | ||
&Event::default(), | ||
None, | ||
&ProjectFiltersConfig::default(), | ||
&GlobalConfig::default() | ||
) | ||
.is_ok()); | ||
} | ||
|
||
#[test] | ||
fn test_expand_profile_with_version_and_segment_id() { | ||
let payload = include_bytes!("../tests/fixtures/sample/v1/segment_id.json"); | ||
assert!(expand_profile(payload, &Event::default()).is_ok()); | ||
assert!(expand_profile( | ||
payload, | ||
&Event::default(), | ||
None, | ||
&ProjectFiltersConfig::default(), | ||
&GlobalConfig::default() | ||
) | ||
.is_ok()); | ||
} | ||
|
||
#[test] | ||
fn test_expand_profile_without_version() { | ||
let payload = include_bytes!("../tests/fixtures/android/legacy/roundtrip.json"); | ||
assert!(expand_profile(payload, &Event::default()).is_ok()); | ||
assert!(expand_profile( | ||
payload, | ||
&Event::default(), | ||
None, | ||
&ProjectFiltersConfig::default(), | ||
&GlobalConfig::default() | ||
) | ||
.is_ok()); | ||
} | ||
} |
Large diffs are not rendered by default.
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.