Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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
5 changes: 5 additions & 0 deletions relay-filter/src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ pub enum FilterStatKey {

/// Filtered due to invalid CSP policy.
InvalidCsp,

/// Filtered due to the fact that it was a call to a health-check endpoint
HealthCheck,
}

// An event grouped to a removed group.
Expand All @@ -153,6 +156,7 @@ impl FilterStatKey {
FilterStatKey::Localhost => "localhost",
FilterStatKey::WebCrawlers => "web-crawlers",
FilterStatKey::InvalidCsp => "invalid-csp",
FilterStatKey::HealthCheck => "health-check",
}
}
}
Expand All @@ -176,6 +180,7 @@ impl<'a> TryFrom<&'a str> for FilterStatKey {
"localhost" => FilterStatKey::Localhost,
"web-crawlers" => FilterStatKey::WebCrawlers,
"invalid-csp" => FilterStatKey::InvalidCsp,
"health-check" => FilterStatKey::HealthCheck,
other => {
return Err(other);
}
Expand Down
12 changes: 12 additions & 0 deletions relay-filter/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ pub struct FiltersConfig {
/// Configuration for the releases filter.
#[serde(default, skip_serializing_if = "ReleasesFilterConfig::is_empty")]
pub releases: ReleasesFilterConfig,

/// Configuration for the healthcheck filter.
#[serde(default, skip_serializing_if = "FilterConfig::is_empty")]
pub health_check: FilterConfig,
}

impl FiltersConfig {
Expand All @@ -223,6 +227,7 @@ impl FiltersConfig {
&& self.legacy_browsers.is_empty()
&& self.localhost.is_empty()
&& self.releases.is_empty()
&& self.health_check.is_empty()
}
}

Expand Down Expand Up @@ -260,6 +265,9 @@ mod tests {
releases: ReleasesFilterConfig {
releases: [],
},
health_check: FilterConfig {
is_enabled: false,
},
}
"###);
Ok(())
Expand Down Expand Up @@ -293,6 +301,7 @@ mod tests {
releases: ReleasesFilterConfig {
releases: GlobPatterns::new(vec!["1.2.3".to_string()]),
},
health_check: FilterConfig { is_enabled: true },
};

insta::assert_json_snapshot!(filters_config, @r###"
Expand Down Expand Up @@ -331,6 +340,9 @@ mod tests {
"releases": [
"1.2.3"
]
},
"healthCheck": {
"isEnabled": true
}
}
"###);
Expand Down
160 changes: 160 additions & 0 deletions relay-filter/src/health_check.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
//! Implements event filtering based on whether the endpoint called is a healthcheck endpoint.
//!
//! If this filter is enabled transactions from healthcheck endpoints will be filtered out.

use once_cell::sync::Lazy;
use regex::Regex;
use relay_general::protocol::Event;

use crate::{FilterConfig, FilterStatKey};

static HEALTH_CHECK_ENDPOINTS: Lazy<Regex> = Lazy::new(|| {
Regex::new(
r#"(?ix)
healthcheck|
healthy|
live|
ready|
heartbeat|
/health$|
/healthz$
"#,
)
.expect("Invalid healthcheck filter Regex")
});
Copy link
Contributor

Choose a reason for hiding this comment

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

How often will this list get updated? Are we planning on new inbound filters on transaction names?

The health check filter checks transaction names, there's nothing specific to health checks besides this pattern. However, if we need to change the pattern, it requires redeploying Relay. Could we benefit from this filter if we make it more generic, and accept a pattern from Sentry? This is the approach with the client IPs inbound filter. This will give us more flexibility. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really know, we had a discussion and it was decided that we'd rather update Relay than add this to the configuration of each project ( I guess the implication is that it will not be updated very often).


fn matches(event: &Event) -> bool {
event.transaction.value().map_or(false, |transaction| {
HEALTH_CHECK_ENDPOINTS.is_match(transaction)
})
}

/// Filters transaction events for calls to healthcheck endpoints
pub fn should_filter(event: &Event, config: &FilterConfig) -> Result<(), FilterStatKey> {
if !config.is_enabled {
return Ok(());
}
if matches(event) {
return Err(FilterStatKey::HealthCheck);
}
Ok(())
}

#[cfg(test)]
mod tests {
use super::*;
use relay_general::protocol::Event;
use relay_general::types::Annotated;

/// tests matching for various transactions
#[test]
fn test_matches() {
let transaction_names = [
"a/b/healthcheck/c",
"a_HEALTHCHECK_b",
"healthcheck",
"a/healthy/b",
"a_healthy_b",
"HEALTHY",
"a/live/b",
"a_live_b",
"live",
"a/READY/b",
"a_ready_b",
"ready",
"a/heartbeat/b",
"a_heartbeat_b",
"heartbeat",
"/health",
"a/HEALTH",
"/healthz",
"a/HEALTHZ",
];

for name in transaction_names {
let event = Event {
transaction: Annotated::new(name.into()),
..Event::default()
};
assert!(matches(&event), "Did not match `{name}`")
}
}

/// tests non matching transactions transactions
#[test]
fn test_does_not_match() {
let transaction_names = [
"bad",
"/bad",
"a/b/c",
"health",
"healthz",
"/health/",
"/healthz/",
"/healthx",
"/healthzx",
];

for name in transaction_names {
let event = Event {
transaction: Annotated::new(name.into()),
..Event::default()
};
assert!(
!matches(&event),
"Did match `{name}` but it shouldn't have."
)
}
}

// test it doesn't match when the transaction name is missing
#[test]
fn test_does_not_match_missing_transaction() {
let event = Event { ..Event::default() };
assert!(
!matches(&event),
"Did match with empty transaction but it shouldn't have."
)
}

#[test]
fn test_filters_when_matching() {
let event = Event {
transaction: Annotated::new("/health".into()),
..Event::default()
};
let filter_result = should_filter(&event, &FilterConfig { is_enabled: true });
assert_eq!(
filter_result,
Err(FilterStatKey::HealthCheck),
"Event did not filter health check event"
)
}
#[test]
fn test_does_not_filter_when_disabled() {
let event = Event {
transaction: Annotated::new("/health".into()),
..Event::default()
};
let filter_result = should_filter(&event, &FilterConfig { is_enabled: false });
assert_eq!(
filter_result,
Ok(()),
"Event filtered although filter should have been disabled"
)
}

#[test]
fn test_does_not_filter_when_not_matching() {
let event = Event {
transaction: Annotated::new("/a/b/c".into()),
..Event::default()
};
let filter_result = should_filter(&event, &FilterConfig { is_enabled: true });
assert_eq!(
filter_result,
Ok(()),
"Event filtered although filter should have not matched"
)
}
}
2 changes: 2 additions & 0 deletions relay-filter/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod browser_extensions;
pub mod client_ips;
pub mod csp;
pub mod error_messages;
pub mod health_check;
pub mod legacy_browsers;
pub mod localhost;
pub mod web_crawlers;
Expand Down Expand Up @@ -56,6 +57,7 @@ pub fn should_filter(
browser_extensions::should_filter(event, &config.browser_extensions)?;
legacy_browsers::should_filter(event, &config.legacy_browsers)?;
web_crawlers::should_filter(event, &config.web_crawlers)?;
health_check::should_filter(event, &config.health_check)?;

Ok(())
}