feat(relay): Implement decaying rule [TET-607] #1692

iambriccardo · 2022-12-13T15:22:35Z

A decaying function is a function that interpolates the value of the sample rate based on a set of parameters (e.g. start, end, fromSampleRate). The rationale behind the decaying function is to be able to systematically change the sample rate based on multiple parameters. This should be especially helpful with the behavior of latest releases, in which the number of transactions grows non immediately and we want to inversely follow that trend.

There exist currently two types of decaying functions:

constant → returns the sampleRate irrespectively of the time of evaluation.
```
{
	...
	"decayingFn": {
		"type": "constant"	
  }
	...
}
```
linear → returns a decreasing sample rate from sampleRate to decayedSampleRate based on the time of evaluation.
```
{ 
  ...
	"decayingFn": {
		"function": "linear",
		"decayedSampleRate": 0.6
	}
	...
}
```

By default, if no decaying function is specified, the constant function will be used, which returns the sampleRate at any point within the specified time range.

Both decaying functions require time ranges, that are fetched from the previously existing timeRange field:

{ 
	...
  "timeRange": {
		"start": "2022-10-21 18:50:25+00:00",
		"end": "2022-10-21 19:50:25+00:00"
	},
	"decayingFn": {
		"function": "linear",
		"decayedSampleRate": 0.6
	}
	...
}

Depending on which decayingFn is used, the timeRange check will be different:

constant → a valid time range can be open or closed (which means that you can have both intervals closed or at most one side of the interval open) and, if both are set, start < end.
linear → a valid time range must be closed and start < end.

In case at least one of the required conditions are not met, the rule with be considered inactive and won’t be matched.

Implementation

The implementation of decaying rules introduces the idea of an ActiveRule which is a rule returned only in case it is active. A rule is active if it satisfies a series of conditions that are checked in relation to the decayingFn used.

An ActiveRule has the get_sample_rate(now) method which returns the sample rate at point in time now.

jjbayer

The general concept makes sense to me, but I have some comments on the code structure.

jjbayer · 2022-12-19T12:31:10Z

relay-sampling/src/lib.rs

+        // We round to 2 digits in order to avoid high-precision sample rates that are more difficult
+        // to work with. Rounding is performed following the nearest integer.
+        let sample_rate_difference =
+            f64::round((from_sample_rate - self.external_params.base_sample_rate) * 100.0) / 100.0;


Why are rounded sample rates easier to work with?

It is easier to debug and in general I thought we were going to keep 2 decimal digits.

By multiplying sample_rate_difference * inverse_progress, you potentially get a lot more decimals again, right? If we want to do 1% steps of decay (e.g. 0.7, 0.69, 0.68, ...) I would round when the computation is done, e.g. in get_sample_rate.

What you are saying is correct but I did that on purpose.

I had at first rounded only the sample difference because it was behaving quite weird, in the sense that I was getting 0.7 - 0.2 = 0.4999999 due to floating arithmetic approximations I think. And this was going to screw up the whole calculation because we started doing that with an ill number.

I think that we could simplify everything and round at the end, with some more loss propagated through multiplications but that is fine. The only problem I see is that we will have weird numbers in tests if we choose for example the 50% progress time.

Do increased precision and floating point glitches really make this more difficult to work with? In the end, you can just make your "correct" calculation and use the result. If you ever have to debug print the value and receive 0.49999..., it is still clear what the value is. Besides, interpolation will give you all sorts of values anyway.

Also, please do not round to integer percentages. We need more precision than that, considering that a realistic uniform sample rate is 1%. You can, of course, round your rule targets, but please do not round here.

I would suggest to remove complexity and not round anywhere in Relay.

Great! Thanks for the explanation Jan!

relay-sampling/src/lib.rs

jjbayer · 2022-12-19T13:20:53Z

relay-sampling/src/lib.rs

+    /// want to inversely reduce the sample rate in the interval (from - base_sample_rate).
+    /// Thus, in the 70% case, we would take only 30% of the (from - base_sample_rate) which
+    /// is equal to 0.06.
+    fn execute_linear_decay(&self, from_sample_rate: f64) -> f64 {


nit: There are variables in this function that begin with start_ and end_, so to clarify intent I would rename from_sample_rate to start_sample_rate, and create an alias let end_sample_rate = self.external_params.base_sample_rate.

jjbayer · 2022-12-19T13:46:20Z

relay-sampling/src/lib.rs

+
+/// A struct containing the set of external params required by a decaying function.
+#[derive(Clone, Copy, Debug)]
+struct DecayingFunctionExternalParams {


I feel that the types involved in this logic are too tightly coupled to the serialization format, in that we differentiate between internal and external params.

In order to decouple them, can we construct something like that from the SamplingRule, e.g.

struct LinearDecay { // whatever model parameters a linear function needs. In theory only two (`(k, d)` for `y = kx + d`), // but it might make sense to store `(start, end)` timestamps and sample rates as well. start_time: DateTime<Utc>, end_time: DateTime<Utc>, start_rate: f64, end_rate: f64, } enum DecayFunc { // ... Linear(LinearDecay), } impl SamplingRule { fn get_decay_fn(&self) -> DecayFunc { // construct DecayFunc from rule here. We can also cache it on the sample rule } }

How would you deserialize this? I wanted to keep my change as small as possible on the existing rule payload.

Your proposal makes total sense, indeed, it is the simplest way of doing it but this requires the removal of the timerange, which we can for sure do but we need to be careful with backward compatibility.

I would leave the existing rule payload as-is, and create these helper structs independently.

This would end up creating duplication within the json. Do we want that? And how do we treat the TimeRange, because it won't be needed anymore if we keep intervals within the functions themselves.

jjbayer · 2022-12-20T11:58:26Z

relay-sampling/src/lib.rs

+///
+/// A decaying function is responsible of decaying the sample rate from a value to another following
+/// a given curve.
+#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]


Suggested change

#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]

#[derive(Default, Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]

relay-sampling/src/lib.rs

jjbayer · 2022-12-20T11:59:21Z

relay-sampling/src/lib.rs

+impl Default for DecayingFunction {
+    fn default() -> Self {
+        Self::Constant
+    }
+}
+


Suggested change

impl Default for DecayingFunction {

fn default() -> Self {

Self::Constant

}

}

jjbayer · 2022-12-20T12:50:57Z

relay-sampling/src/lib.rs

+        if let Some(params) = self.get_decaying_function_params(now) {
+            self.decaying_fn.call(params)
+        } else {
+            self.sample_rate


Now that sample_rate is not the base sample rate anymore, I believe that instead of falling back to this value we should make sure that invalid rules do not match any events. We already have the is_active mechanism for that, should we extend that function so it returns false for invalid rules?

relay/relay-sampling/src/lib.rs

Lines 741 to 742 in 0e8bd22

if !rule.is_active() {

return false;

@jan-auer this is a good point, with the new implementation, we would need to decide what to do in case of invalid params. We could either:

Implement a mechanism that automatically falls back either to decayedSampleRate or sampleRate based on the function type.

Make automatically the rule invalid by performing a validation of params before get_sample_rate

Propagate some error but this is generally not suggested and looking at the current implementation it is not done

Just to clarify, this can happen for example if timeRange has start and not end, irrespectively if we specify a decaying function or not.

We could write a custom deserializer for SamplingConfig::rules, which removes structurally invalid rules and logs an error for them. But get_decaying_function_params also depends on now, and that part of the "validation" should go into is_active IMO.

I think that first we would need to clarify whether we want to allow open intervals or not.

If we wanted to do it better, I would have redefined the timerange with only closed intervals and then automatically use the constant decaying if not specified. Then for each function build custom extra validation and fallback mechanism(s).

I don't know tho, if there is a proper rationale behind the decision of open intervals. For DS we don't need it (as of now).

relay-sampling/src/lib.rs

relay-server/src/utils/dynamic_sampling.rs

relay-sampling/src/lib.rs

jjbayer · 2022-12-21T11:46:33Z

CHANGELOG.md


 - Add support for `limits.keepalive_timeout` configuration. ([#1645](https://github.com/getsentry/relay/pull/1645))
+- Add support for decaying functions in sampling rules. ([#1692](https://github.com/getsentry/relay/pull/1692))


Suggested change

- Add support for decaying functions in sampling rules. ([#1692](https://github.com/getsentry/relay/pull/1692))

- Add support for decaying functions in dynamic sampling rules. ([#1692](https://github.com/getsentry/relay/pull/1692))

jjbayer · 2022-12-21T11:47:55Z

relay-sampling/src/lib.rs

@@ -427,57 +399,95 @@ impl SamplingRule {
    ///


Let's update this doc comment.

^ One line above, it says Returns whether the sampling rule is active. We should change it to "Returns an ActiveRule if the rule is active", or something like that.

jjbayer · 2022-12-21T11:54:08Z

relay-sampling/src/lib.rs

+                    end: Some(end),
+                } = self.time_range
+                {
+                    if self.sample_rate > decayed_sample_rate && start < end && now >= start {


Because time_range.contains is not called in this branch anymore, we need to also check that now is < end (feel free to change <= to < and vice versa).

Suggested change

if self.sample_rate > decayed_sample_rate && start < end && now >= start {

if self.sample_rate > decayed_sample_rate && start <= now && now < end {

We do not need to check start < end explicitly then, because it follows from start <= now < end.

Apparently this was not caught by any test case. Maybe we should add one?

Actually this was done with a purpose but for the previous implementation, now we should explicitly verify like you said because the check is in is_active.

jjbayer · 2022-12-21T11:55:09Z

relay-sampling/src/lib.rs

+/// A sampling rule that has been successfully matched and that contains all the required data
+/// to return the sample rate.
+#[derive(Debug, Clone, Copy)]
+pub struct MatchingRule {


ActiveRule might actually be a better name now that I think of it, because it is created by is_active.

Suggested change

pub struct MatchingRule {

pub struct ActiveRule {

jjbayer · 2022-12-21T11:57:34Z

relay-sampling/src/lib.rs

-            DecayingFunction::Linear {
+/// A struct representing the evaluation context of a sample rate.
+#[derive(Debug, Clone, Copy)]
+enum SampleRateEvaluator {


We usually declare types before using them, so In this case, declare SampleRateEvaluator, then MatchingRule, then SamplingRule. See internal docs.

jjbayer

Couple of nitpicks, apart from those this looks good to me!

jjbayer · 2022-12-21T13:20:10Z

relay-sampling/src/lib.rs

@@ -427,57 +399,95 @@ impl SamplingRule {
    ///


^ One line above, it says Returns whether the sampling rule is active. We should change it to "Returns an ActiveRule if the rule is active", or something like that.

jjbayer · 2022-12-21T13:23:47Z

relay-server/src/utils/dynamic_sampling.rs

+    // For consistency reasons we take a snapshot in time and use that time across all code that
+    // requires it.
+    let now = Utc::now();


jjbayer · 2022-12-21T13:29:27Z

relay-sampling/src/lib.rs

+    pub fn get_sample_rate(&self, now: DateTime<Utc>) -> f64 {
+        self.evaluator.evaluate(now)
+    }
+}


nit: Because SamplingRule uses active rule, I would change the declaration order to

SampleRateEvaluator ActiveRule SamplingRule

jjbayer · 2022-12-21T13:39:40Z

relay-server/src/utils/dynamic_sampling.rs

        );

        assert_eq!(
            prepare_and_get_sampling_rule(1.0, EventType::Transaction, &project_state, now)
                .unwrap()
                .unwrap()
                .sample_rate,
-            0.7
+            0.44999999999999996


This looks flaky, should we rather do something like (expected - result).abs() < eps instead? See https://rust-lang.github.io/rust-clippy/master/#float_cmp

Very good call! I totally overlooked the precision, especially considering that it can differ from architecture to architecture.

iambriccardo added 4 commits December 13, 2022 16:22

feat(relay): Implement decaying rule

95d237e

Improve implementation

b965835

Improve implementation

681e3b5

Add tests

020b68b

iambriccardo changed the title ~~feat(relay): Implement decaying rule~~ feat(relay): Implement decaying rule [TET-607] Dec 14, 2022

iambriccardo marked this pull request as ready for review December 14, 2022 13:53

iambriccardo requested review from a team, jan-auer and iker-barriocanal December 14, 2022 13:53

iambriccardo added 4 commits December 14, 2022 15:20

Add documentation

a5f3f0e

Merge branch 'master' into riccardo/decaying-function

b229a71

Rework implementation with internal tagging

7dceff6

Improve code

62949ec

iambriccardo requested a review from jjbayer December 19, 2022 12:43

jjbayer reviewed Dec 19, 2022

View reviewed changes

Simplify implementation

85f2a57

iambriccardo requested a review from jjbayer December 20, 2022 10:52

jjbayer reviewed Dec 20, 2022

View reviewed changes

iambriccardo added 5 commits December 20, 2022 16:02

Fix PR comments

2154a67

Update CHANGELOG

cba7321

Merge branch 'master' into riccardo/decaying-function

2d996e8

Rework implementation

2f5dd94

Improve implementation

02fefb8

jjbayer reviewed Dec 21, 2022

View reviewed changes

Fix PR comments

d05f690

iambriccardo requested a review from jjbayer December 21, 2022 12:27

jjbayer approved these changes Dec 21, 2022

View reviewed changes

iambriccardo added 3 commits January 4, 2023 09:37

Fix PR comments

00a9c92

Merge branch 'master' into riccardo/decaying-function

62f8aa8

Fix PR issues

11e7432

jjbayer approved these changes Jan 9, 2023

View reviewed changes

iambriccardo merged commit cc87076 into master Jan 9, 2023

iambriccardo deleted the riccardo/decaying-function branch January 9, 2023 10:26

olksdr pushed a commit that referenced this pull request Jan 11, 2023

feat(relay): Implement decaying rule [TET-607] (#1692)

1e10617

This was referenced Jan 16, 2023

feat(dynamic-sampling): Add decaying function support for latest release boost [TET-624] getsentry/sentry#43300

Merged

feat(dynamic-sampling): Add decaying function support to sentry [TET-624] getsentry/sentry#43376

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(relay): Implement decaying rule [TET-607] #1692

feat(relay): Implement decaying rule [TET-607] #1692

iambriccardo commented Dec 13, 2022 •

edited

Loading

jjbayer left a comment

jjbayer Dec 19, 2022

iambriccardo Dec 19, 2022

jjbayer Dec 19, 2022

iambriccardo Dec 20, 2022

jan-auer Dec 20, 2022

iambriccardo Dec 20, 2022

jjbayer Dec 19, 2022

jjbayer Dec 19, 2022

iambriccardo Dec 19, 2022 •

edited

Loading

iambriccardo Dec 19, 2022

jjbayer Dec 19, 2022

iambriccardo Dec 20, 2022

jjbayer Dec 20, 2022

jjbayer Dec 20, 2022

jjbayer Dec 20, 2022

iambriccardo Dec 20, 2022

jjbayer Dec 20, 2022

iambriccardo Dec 20, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

iambriccardo Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

iambriccardo Dec 21, 2022

jjbayer left a comment

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

jjbayer Dec 21, 2022

iambriccardo Jan 4, 2023

	#[derive(Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]
	#[derive(Default, Debug, Clone, Copy, PartialEq, Serialize, Deserialize)]


		- Add support for `limits.keepalive_timeout` configuration. ([#1645](https://github.com/getsentry/relay/pull/1645))
		- Add support for decaying functions in sampling rules. ([#1692](https://github.com/getsentry/relay/pull/1692))

	if self.sample_rate > decayed_sample_rate && start < end && now >= start {
	if self.sample_rate > decayed_sample_rate && start <= now && now < end {

feat(relay): Implement decaying rule [TET-607] #1692

feat(relay): Implement decaying rule [TET-607] #1692

Conversation

iambriccardo commented Dec 13, 2022 • edited Loading

Implementation

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iambriccardo commented Dec 13, 2022 •

edited

Loading

iambriccardo Dec 19, 2022 •

edited

Loading