-
Notifications
You must be signed in to change notification settings - Fork 99
feat(filter): Add a health check filter #2118
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
Conversation
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.
The PR looks good, but I want to clarify the configurability of the health check pattern from Sentry, see the comment below.
relay-filter/src/health_check.rs
Outdated
static HEALTH_CHECK_ENDPOINTS: Lazy<Regex> = Lazy::new(|| { | ||
Regex::new( | ||
r#"(?ix) | ||
healthcheck| | ||
healthy| | ||
live| | ||
ready| | ||
heartbeat| | ||
/health$| | ||
/healthz$ | ||
"#, | ||
) | ||
.expect("Invalid healthcheck filter Regex") | ||
}); |
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 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?
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.
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).
Co-authored-by: Iker Barriocanal <[email protected]>
Co-authored-by: Iker Barriocanal <[email protected]>
Co-authored-by: Iker Barriocanal <[email protected]>
Co-authored-by: Iker Barriocanal <[email protected]>
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.
The config will live in Relay for now.
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.
Blocking for naming inconsistencies.
relay-filter/src/config.rs
Outdated
default, | ||
skip_serializing_if = "HealthCheckEndpointsFilterConfig::is_empty" | ||
)] | ||
pub health_check: HealthCheckEndpointsFilterConfig, |
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.
This filter works for any pattern, not just health checks. Could we rename it to reflect it (e.g. transaction_names
)? It's ok if we just use it for health checks, but once we introduce health_check
we should keep it for backward compatibility.
This PR implements a filter for health-check events
Relates to getsentry/sentry#49082