Conversation
| // - as_i64 is not really fast, but most values in sampling rules can be i64, so we could | ||
| // return early | ||
| // - f64 is more likely to succeed than u64, but we might lose precision | ||
| if let (Some(a), Some(b)) = (value.as_i64(), self.value.as_i64()) { |
There was a problem hiding this comment.
I think this should be fine but tbh i'm not super confident. I'm trying to think of cases where both values don't have decimal points, but can't be cast to the same int type:
e.g.: self.value is negative, event value is u64::MAX (self.value can't be cast to u64, and event value can't be cast to i64)
but i think in those cases we should be fine with casting those values to floats for comparison...
jjbayer
left a comment
There was a problem hiding this comment.
This is pretty awesome! I have some minor comments, but nothing that should keep you from merging.
| // XXX(slow): this is a double-for-loop, but we extract like 6 metrics per transaction | ||
| for metric in &mut *metrics { | ||
| if !rule.target_metrics.contains(&metric.name) | ||
| || metric.tags.contains_key(&rule.target_tag) |
There was a problem hiding this comment.
Would add comment here to explain why we skip if the tag already exists.
relay-sampling/src/lib.rs
Outdated
| } | ||
| _ => Value::Null, | ||
| }, | ||
| x if x.starts_with("transaction.measurements.") => { |
There was a problem hiding this comment.
nit
| x if x.starts_with("transaction.measurements.") => { | |
| field_name if field_name.starts_with("transaction.measurements.") => { |
| impl_cmp_condition!(GteCondition, >=); | ||
| impl_cmp_condition!(LteCondition, <=); | ||
| impl_cmp_condition!(LtCondition, <); | ||
| impl_cmp_condition!(GtCondition, >); |
There was a problem hiding this comment.
Out of curiosity, would there be a way to do this without a macro? Something like type GtCondition = CmpCondition<std::cmp::PartialOrd::gt>. I guess you would run into problems because you have three different operand types i64, u64, f64 inside?
There was a problem hiding this comment.
I didn't find a way, you'd have to be generic over a type, and functions are either:
- a singular type
fn(a: i64, b: i64) -> bool(regular functions) - an unnameable type (closures)
there's no const generics for function pointers yet
In order to make generics truly work, you'd have to define a trait Comparator, implement it for a few zero-sized types Lt, Gt, ...:
trait Comparator {
fn matches(a: Number, b: Number) -> bool;
}
struct Lt;
impl Comparator for Lt { ... }
struct CmpCondition<A> { ... }
impl<A: Comparator> CmpCondition<A> {
fn matches(self, event: ...) -> bool {
A::matches(self.number, get_event_number(event))
}
}
type LtCondition = CmpCondition<Lt>;
but this seemed like more effort
I think instead of being generic over a type what you could do is to write a single non-generic type:
struct CmpCondition {
cmp_fn: fn(a: Number, b: Number) -> bool;
...
}
but then you have to write custom deserialization logic, and serialization just won't work unless you keep the enum variants:
enum RuleCondition {
Gte(CmpCondition),
Lt(CmpCondition)
}
| "tagValue": "tolerated" | ||
| }, | ||
| { | ||
| "condition": {"op": "and", "inner": []}, |
There was a problem hiding this comment.
This is basically the else branch, right?
There was a problem hiding this comment.
yup. i felt that we wouldn't need an always-true condition since we can already write this today
In getsentry/relay#1225 we introduced a general-purpose tagging system for transaction metrics. This may later be extended: * we will add outlier tagging for histograms soon * session metrics could be tagged the same way (unlikely to happen for now, needs changes in Relay) Here we move away from the purpose-built user satisfaction impl to the more generic one
…ogram outliers (#33722) * ref(mep): Move transaction thresholds to new tagging system In getsentry/relay#1225 we introduced a general-purpose tagging system for transaction metrics. This may later be extended: * we will add outlier tagging for histograms soon * session metrics could be tagged the same way (unlikely to happen for now, needs changes in Relay) Here we move away from the purpose-built user satisfaction impl to the more generic one * Update src/sentry/relay/config/__init__.py * Update src/sentry/relay/config/metric_extraction.py Co-authored-by: Joris Bayer <joris.bayer@sentry.io> * apply review feedback * style(lint): Auto commit lint changes * style(lint): Auto commit lint changes * update rule fields * Add histogram outliers code * fix outlier detection * update snapshots * fix typing * remove useless lines * fixate orderby Co-authored-by: Joris Bayer <joris.bayer@sentry.io> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
As of getsentry/sentry#33722, we do not send the `satisfactionThresholds` project config key anymore. See diff that removes the config [here](https://github.com/getsentry/sentry/pull/33722/files#diff-5e30755537fcfd5687d4399272065e49affcc12a4cf7a0bc377cff5234b8a08a). Satisfaction thresholds are now applied using the more generic [conditional tagging](#1225) mechanism. This PR removes all traces of the old configuration and business logic.
We're building this such that we can supersede certain kinds of duration-range based tagging we do in metrics extraction right now.
https://www.notion.so/sentry/Metrics-outlier-filtering-in-Relay-93c1a9a8c6c24277a2c9e8d61876f968?d=d1035fca98804ef697997439d14a06b2#3e08fc7f89a24de8a8c85b7d717d35f6=