chore(spool): Enable periodic unspool #2993

olksdr · 2024-01-23T17:15:35Z

Currently in order to trigger unspool of buffered envelopes the new project state update must be received from the upstream, which sometimes can take longer time.

To make unspool action a bit more predictable let's enable periodic unspool initiated by project cache. This way:

spool service does not have to know when to unspool
spool service just has to serve the request and communicate back the results
project cache will take care of the initiating the action and only queue the envelopes when certain criteria are met

part of: https://github.com/getsentry/team-ingest/issues/267

relay-config/src/config.rs

relay-server/src/services/project_cache.rs

Dav1dde · 2024-01-24T08:11:15Z

relay-server/src/services/project_cache.rs

+
+        // If there is nothing spooled, schedule the next check a little bit later.
+        if keys.is_empty() {
+            self.schedule_unspool();


Is this to schedule the unspool before resetting?

Can we return in this branch to make it clear, e.g.

self.schedule_unspool(); self.buffer_unspool_backoff.reset(); return;

nit: Should the reset of the backoff be part of schedule_unspool?

nope, since if there is nothing to unspool, I want to wait a little bit longer before trying to unspool again. Once the unspool finishes with anything, we reset the backoff to initial state (which currently 100ms)

relay-server/src/services/project_cache.rs

jjbayer · 2024-01-24T10:26:02Z

relay-server/src/services/project_cache.rs

@@ -913,6 +895,48 @@ impl ProjectCacheBroker {
        }
    }

+    /// Returns backoff timeout for an unspool attempt.
+    fn next_unspool_attempt(&mut self) -> Duration {
+        self.config.spool_envelopes_unspool_interval() + self.buffer_unspool_backoff.next_backoff()


nit: Instead of adding an interval to a backoff, could we use ExponentialBackoff instead of RetryBackoff and initialize it with an initial_interval of self.config.spool_envelopes_unspool_interval()?

do you mean to use backoff::ExponentialBackoff directly?

We have a little better interface and we also could add some functions to make the setting of things for it more convenient.

Makes sense. We could make RetryBackoff more flexible (i.e. accept a custom initial delay), but that's out of scope for this PR.

relay-server/src/services/project_cache.rs

jjbayer · 2024-01-24T10:37:12Z

relay-server/src/services/project_cache.rs

+
+        // If there is nothing spooled, schedule the next check a little bit later.
+        if keys.is_empty() {
+            self.schedule_unspool();


nit: Should the reset of the backoff be part of schedule_unspool?

jjbayer · 2024-01-24T10:39:48Z

relay-server/src/services/project_cache.rs

+            if self
+                .projects
+                .get(project_key)
+                .map_or(false, |state| state.valid_state().is_some())


Is the project config actually being refreshed anywhere now? In the previous impl we had .get_cached_state(project_cache, false) which did an implicit fetch.

It will be refreshed as it was before like normal refresh operation.
Here I just want to check for the valid states, and if there are try to unspool them.

Gotcha. It might not be related to the goal of this PR but I feel like we should trigger a prefetch at this point. Otherwise we still depend on incoming traffic to get old data unspooled, right?

it's a valid point.
I've switched to using get_cached_state instead, which will trigger the refresh of the project state if it's not valid or expired.

iker-barriocanal · 2024-01-24T14:06:16Z

relay-server/src/services/project_cache.rs

@@ -683,8 +663,10 @@ impl ProjectCacheBroker {
            no_cache,
        );

-        if !state.invalid() {
-            self.dequeue(project_key);
+        // Schedule unspool if nothing is running at the moment.


Should we check here too whether there are enough buffer guards available?

nope, the check is done in handle_periodic_unspool now.

jjbayer · 2024-01-24T15:10:25Z

relay-server/src/services/project_cache.rs

@@ -913,6 +895,48 @@ impl ProjectCacheBroker {
        }
    }

+    /// Returns backoff timeout for an unspool attempt.
+    fn next_unspool_attempt(&mut self) -> Duration {
+        self.config.spool_envelopes_unspool_interval() + self.buffer_unspool_backoff.next_backoff()


Makes sense. We could make RetryBackoff more flexible (i.e. accept a custom initial delay), but that's out of scope for this PR.

jjbayer · 2024-01-24T16:35:07Z

relay-server/src/services/project_cache.rs

+            if self
+                .projects
+                .get(project_key)
+                .map_or(false, |state| state.valid_state().is_some())


Gotcha. It might not be related to the goal of this PR but I feel like we should trigger a prefetch at this point. Otherwise we still depend on incoming traffic to get old data unspooled, right?

CHANGELOG.md

olksdr · 2024-01-25T08:50:50Z

relay-server/src/services/project_cache.rs

@@ -1268,4 +1309,89 @@ mod tests {
            assert!(buffer_rx.try_recv().is_err())
        }
    }
+
+    #[tokio::test]
+    async fn periodic_unspool() {


@jjbayer Added a test, if you want to have a look.

chore(spool): Enable periodic unspool

fa106b4

olksdr self-assigned this Jan 23, 2024

olksdr requested a review from a team as a code owner January 23, 2024 17:15

Add CHANGELOG entry

0e26205

Dav1dde reviewed Jan 24, 2024

View reviewed changes

jjbayer reviewed Jan 24, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into chore/unspool-re-tune

0f37d11

iker-barriocanal reviewed Jan 24, 2024

View reviewed changes

olksdr added 2 commits January 24, 2024 15:23

review comments: moved checking permits watermark in handle_unspool

10af52f

Merge remote-tracking branch 'origin/master' into chore/unspool-re-tune

5a7d737

olksdr requested review from jjbayer, iker-barriocanal, Dav1dde and a team January 24, 2024 14:40

olksdr added 2 commits January 24, 2024 15:48

move checks around

9543d75

Changelog

cf25609

jjbayer approved these changes Jan 24, 2024

View reviewed changes

olksdr added 3 commits January 25, 2024 07:19

Merge remote-tracking branch 'origin/master' into chore/unspool-re-tune

7921c59

review: check and refresh project state in one go

78acb5e

fix docs

becee2c

Dav1dde approved these changes Jan 25, 2024

View reviewed changes

Add tests for periodic unspool

2236b1d

olksdr commented Jan 25, 2024

View reviewed changes

olksdr added 2 commits January 25, 2024 10:03

cleanup

5a6755c

add backoff to unspool checks

1bedf3b

olksdr merged commit c892dd9 into master Jan 25, 2024
20 checks passed

olksdr deleted the chore/unspool-re-tune branch January 25, 2024 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(spool): Enable periodic unspool #2993

chore(spool): Enable periodic unspool #2993

olksdr commented Jan 23, 2024 •

edited

Loading

Dav1dde Jan 24, 2024

jjbayer Jan 24, 2024

olksdr Jan 24, 2024

jjbayer Jan 24, 2024

olksdr Jan 24, 2024

jjbayer Jan 24, 2024

jjbayer Jan 24, 2024

jjbayer Jan 24, 2024

olksdr Jan 24, 2024

jjbayer Jan 24, 2024

olksdr Jan 25, 2024

iker-barriocanal Jan 24, 2024

olksdr Jan 24, 2024

jjbayer Jan 24, 2024

jjbayer Jan 24, 2024

olksdr Jan 25, 2024

jjbayer Jan 25, 2024

chore(spool): Enable periodic unspool #2993

chore(spool): Enable periodic unspool #2993

Conversation

olksdr commented Jan 23, 2024 • 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

olksdr commented Jan 23, 2024 •

edited

Loading