feat(server): Add inbound filters functionality to dynamic sampling #920

RaduW · 2021-02-05T09:19:00Z

Adds dynamic sampling rules to support inbound filters functionality.

Covering the inbound filters functionality with dynamic sampling rules allows us to remove
inbound filters and present the user with only one concept and one configuration interface.

now we have matches_trace & matches_event

…into feat/dyn-sampling-filters

`parsed.sequence - default_sequence()` is always <= 0 when it is < 0 we get a panic since the value is unsigned (u64)

…ilters # Conflicts: # CHANGELOG.md

jan-auer

Partial review of just the filter changes.

CHANGELOG.md

relay-filter/src/browser_extensions.rs

relay-filter/src/client_ips.rs

relay-filter/src/config.rs

relay-filter/src/csp.rs

relay-general/src/protocol/session.rs

relay-server/src/utils/dynamic_sampling.rs

jan-auer · 2021-02-25T14:28:09Z

relay-server/src/utils/dynamic_sampling.rs

@@ -127,8 +201,11 @@ impl NotCondition {
    fn supported(&self) -> bool {
        self.inner.supported()
    }
-    fn matches<T: FieldValueProvider>(&self, value_provider: &T) -> bool {
-        !self.inner.matches(value_provider)
+    fn matches_event(&self, event: &Event, ip_addr: Option<IpAddr>) -> bool {


In a follow-up, let's replace all these ip_addr with RequestMeta:

It's to be expected that we require more information about the request

ip_addr doesn't specify which IP we want: the socket, the client, how to handle x-forwarded-for? This is what RequestMeta already solves for us.

jan-auer · 2021-02-25T14:28:52Z

relay-server/src/utils/dynamic_sampling.rs

+pub struct CustomCondition {
+    pub name: String,
+    #[serde(default)]
+    pub value: Value,


If we have generic options, why do we need value here?

For a follow-up: It would be interesting to type out the custom conditions, since they are statically known in the code.

to have a consistent structure that matches with the non custom operators.
For example:

{ "name": "event.releases", "op": "glob", "value": ["v1.1", "v2.*"]}, { "name": "event.legacy_browsers", "op":"custom", "value":["ie8", "ie9"]},

The options are supposed to be used for configuring the operator (something like the ignore_case from operator eq ).

Understood. Once these are typed out it certainly looks better again. Let's keep like this, then.

On another thought: It seems odd that we have options but name and value special cased on the top, once everything is typed out. We can address this when typing out, though.

jan-auer · 2021-02-25T14:33:41Z

relay-server/src/utils/dynamic_sampling.rs

+    /// returns a match result
+    fn get_custom_operator(
+        name: &str,
+    ) -> fn(condition: &CustomCondition, slf: &Self, ip_addr: Option<IpAddr>) -> bool;


Probably also for a follow-up: Can we make it so that the Field provider inherently has access to the ip_addr (or, better, the entire RequestMeta)? We could implement the trait on a type like this:

struct EventValueProvider<'a> { event: &'a Annotated<Event>, meta: &'a RequestMeta, }

jan-auer · 2021-02-25T14:35:50Z

relay-server/src/utils/dynamic_sampling.rs

+    /// returns a match result
+    fn get_custom_operator(
+        name: &str,
+    ) -> fn(condition: &CustomCondition, slf: &Self, ip_addr: Option<IpAddr>) -> bool;


Q: Why not just run the custom condition inside the function?

fn match_custom(&self, condition: &CustomCondition, ip_addr: Option<IpAddr>) -> bool;

For a follow-up: Later down the road we need to refactor the value provider so that it can expose all fields needed by the custom rules via its get_value interface.

We could do that, it seemed wrong though.
The ValueProvider gives you information from the Event or Trace but doesn't also run
rules, anyway not feeling too strongly either way.
The presence of the custom operation in ValueProvider makes it quite a messy abstraction.

Separate from this PR, could you compile a list of all attributes that we need access to for running the custom matchers?

relay-server/src/utils/dynamic_sampling.rs

jan-auer · 2021-02-26T08:33:34Z

relay-server/src/utils/dynamic_sampling.rs

        }
    }

+    #[test]
+    /// test matching for various rules
+    fn test_matches_events() {


In a follow-up, I would like to move all these tests to macro-based expansion. That way, each test runs in its own test function and we can see individual failures more easily.

We could also start adopting a test parameterization framework, but I'm still not convinced of that.

While I like the idea in principle it would make debugging difficult/impossible and I like to have this option open.
I've been looking, a while back now, for a clean way to do parameterization of unit tests in Rust but all I found were frameworks that either used nightly builds or used some ugly macros.

If we can find a relatively clean way to do it while still being able to run a debugger I all for it.

You can run and debug tests defined in macros just like any other test. Rust Analyzer even provides code lenses for debugging individual tests, for example as we do in relay-general/tests/test_fixtures.rs:

Co-authored-by: Jan Michael Auer <[email protected]>

* master: (23 commits) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog meta(readme): Document development config for processing mode (#921) ...

* master: (42 commits) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) feat(protocol): Add NSError to mechanism (#925) meta: Fix changelog ...

* master: (120 commits) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930) build: drop python 2.7 support (#929) feat(protocol): Add snapshot to the stack trace interface (#927) test: Improve integration tests to reduce flakyness (#928) fix: Ignore false-positive lints from Clippy 1.50 (#926) ...

* master: test(general): Introduce macros for traversing Annotated (#826) fix(protocol): Deny backslashes in release names (#904) ref(server): Extract dynamic sampling functionality into own crate (#940) build: Switch to vendored openssl build (#914) feat(server): Add inbound filters functionality to dynamic sampling (#920) fix rule generation to new format (in tests) (#939) ref: Remove actix-web client, make reqwest the new http client (#938) fix(server): Reject empty envelopes with an explicit error (#937) fix(server): Ensure outcome data categories are numeric (#936) feat(stats): Add data category to TrackOutcome (#931) fix(pii): Make username pii-strippable (#935) build: Bump symbolic to 8.0.4 (#899) fix: Update to tokio 1.0 (#909) fix(pii): Make and/or work correctly with pii=maybe (#932) release: 21.2.0 release: 0.8.3 fix(build): Fix path to 3.7 (#930)

RaduW added 6 commits February 2, 2021 12:17

add browser extensions and localhost to dyn sampling filters

7a73ec3

changed rule matching to type specific implementations

6533fcc

now we have matches_trace & matches_event

added legacy browser dynamic filter

77fe904

added csp dynamic filter

1e68a4c

added error messages filter

92e95ae

added tests for ported inbound filters into dynamic sampling rules.

8fe4040

RaduW requested a review from a team February 5, 2021 09:19

RaduW marked this pull request as draft February 5, 2021 09:19

RaduW added 2 commits February 5, 2021 10:22

add PR description to changelog

9309ce2

add client_ip rule

9d05f1c

RaduW requested a review from jan-auer February 8, 2021 10:15

beezz and others added 3 commits February 9, 2021 13:28

librdkafka use only v4 for producer

3486015

Merge remote-tracking branch 'origin/integration-tests-kafka-update' …

1b0eee5

…into feat/dyn-sampling-filters

small fix - intermittent failure

19c888f

`parsed.sequence - default_sequence()` is always <= 0 when it is < 0 we get a panic since the value is unsigned (u64)

RaduW marked this pull request as ready for review February 11, 2021 12:33

RaduW added 8 commits February 11, 2021 13:40

merge master

54ecee5

Merge branch 'master' into feat/dyn-sampling-filters

bb66229

added new (uniform) serialization format for dynamic filters

5eccfbb

Merge remote-tracking branch 'origin/master' into feat/dyn-sampling-f…

f677a1e

…ilters # Conflicts: # CHANGELOG.md

added missing unit tests for dynamic filters

0a14c76

manually reverting "librdkafka use only v4 for producer" 3486015

a9dce34

merge master

d07406e

minor, fix naming consistency

8a85e43

jan-auer reviewed Feb 25, 2021

View reviewed changes

RaduW added 2 commits February 25, 2021 11:56

fixes from code review

a122319

fix rule generation to new format (in tests)

339b848

getsentry deleted a comment from RaduW Feb 25, 2021

Merge branch 'master' into feat/dyn-sampling-filters

7e5ec74

jan-auer reviewed Feb 25, 2021

View reviewed changes

clenup lint errors

93e8b20

RaduW added 2 commits February 25, 2021 15:59

minor, imports cleanup

2e3fc31

changed LegacyBrowser trait from From<&str> to FromStr

0c1cc64

RaduW requested a review from jan-auer February 25, 2021 16:00

jan-auer approved these changes Feb 26, 2021

View reviewed changes

RaduW and others added 3 commits February 26, 2021 09:58

Update relay-server/src/utils/dynamic_sampling.rs

82df5db

Co-authored-by: Jan Michael Auer <[email protected]>

lint rust code

827737d

minor, dependency cleanup

ebf3c94

RaduW merged commit 4b32791 into master Feb 26, 2021

RaduW deleted the feat/dyn-sampling-filters branch February 26, 2021 09:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(server): Add inbound filters functionality to dynamic sampling #920

feat(server): Add inbound filters functionality to dynamic sampling #920

RaduW commented Feb 5, 2021 •

edited

Loading

jan-auer left a comment

jan-auer Feb 25, 2021

jan-auer Feb 25, 2021

RaduW Feb 25, 2021

jan-auer Feb 25, 2021

jan-auer Feb 25, 2021

jan-auer Feb 25, 2021

jan-auer Feb 25, 2021

RaduW Feb 25, 2021

jan-auer Feb 25, 2021

jan-auer Feb 26, 2021

RaduW Feb 26, 2021

jan-auer Feb 26, 2021

feat(server): Add inbound filters functionality to dynamic sampling #920

feat(server): Add inbound filters functionality to dynamic sampling #920

Conversation

RaduW commented Feb 5, 2021 • edited Loading

jan-auer 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

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

RaduW commented Feb 5, 2021 •

edited

Loading