feat(filter): Add a health check filter#2118
Conversation
iker-barriocanal
left a comment
There was a problem hiding this comment.
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.
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.
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 <32816711+iker-barriocanal@users.noreply.github.com>
Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
Co-authored-by: Iker Barriocanal <32816711+iker-barriocanal@users.noreply.github.com>
iker-barriocanal
left a comment
There was a problem hiding this comment.
The config will live in Relay for now.
iker-barriocanal
left a comment
There was a problem hiding this comment.
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.
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