-
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
Merged
Zylphrex
merged 11 commits into
master
from
txiao/feat/support-inbound-filters-for-profiles
Oct 30, 2024
Merged
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
f230001
feat(profiling): Support inbound filters for profiles
Zylphrex 48ccc55
Merge branch 'master' of github.com:getsentry/relay into txiao/feat/s…
Zylphrex f56cc46
update changelog
Zylphrex f35cffb
lint
Zylphrex ea2856e
Merge branch 'master' of github.com:getsentry/relay into txiao/feat/s…
Zylphrex 5b0798d
fix tests
Zylphrex 54097ea
lint
Zylphrex 9b499c9
add tests
Zylphrex 0ca0fc1
add tests
Zylphrex d097ec6
use smaller android profile
Zylphrex 696edb4
Merge branch 'master' into txiao/feat/support-inbound-filters-for-pro…
Zylphrex File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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_ref().map(|release| release.as_str()) | ||
Zylphrex marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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_ref().map(|release| release.as_str().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()); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.