ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] #1508

olksdr · 2022-10-02T11:22:48Z

Let's convert Metric Aggregator actor to tokio runtime.

In this PR:

Create AggregatorService which runs on tokio runtime
Some cleanup of the old messages and handlers
Make sure all the Rust tests are working

Some open questions:

how to test the new service when the both old and new runtime is required - like handling the shutdown message from the. controller?
currently, just to keep changes to minimum, I use following
```
 if let Some(registry) = REGISTRY.get() {
                registry
                    .aggregator
                    .send(InsertMetrics::new(self.project_key, metrics))
            }
```
to get the aggregator from the registry and then send the message to it. This must be done better - share the channels with dependants?

flub

It seems there's still an lot of actix code left in here? Is that intentional?

relay-metrics/src/aggregation.rs

flub · 2022-10-03T10:45:15Z

relay-metrics/src/aggregation.rs

        let receiver = TestReceiver::default();
+        // Create the new tokio runtime where we start AggregatorService
+        let rt = tokio::runtime::Runtime::new().unwrap();
+        // Run in the old actix system for receiver


Do we really still need the old runtime for this test? Can we not convert TestReceiver to tokio as well?

We could, but in production we still have project_cache actor which uses old system and we still have to test it.
Once we convert that actor to the new tokio, we can change all of this as well.

sure, but these tests are to make sure the aggregation actor works i think, not to make sure that the callers/senders mess something up?

relay-metrics/src/aggregation.rs

flub · 2022-10-03T10:53:11Z

relay-metrics/src/aggregation.rs

+    iter::{FromIterator, FusedIterator},
+    mem,
+    time::{Duration, Instant},
+};

 use actix::prelude::*;


This should be removed, should it not?

flub · 2022-10-03T10:55:12Z

relay-metrics/src/aggregation.rs

@@ -1388,15 +1437,16 @@ impl<T: Iterator<Item = Bucket>> FusedIterator for CappedBucketIter<T> {}
 /// all elapsed buckets belonging to the same [`ProjectKey`] are flushed together.
 ///
 /// Receivers must implement a handler for the [`FlushBuckets`] message.
-pub struct Aggregator {
+pub struct AggregatorService {
    config: AggregatorConfig,
    buckets: HashMap<BucketKey, QueuedBucket>,
    receiver: Recipient<FlushBuckets>,


Recipient is still something actix, why is that still here?

This service still interacts with project_cache actor which runs on the old actix. I wanted to keep these changes to the minimum and then when the project_cache is converted we can remove this Recipient and maybe replace it with mpsc channel?

I can look into it and maybe introduce changes in this PR, if that's better for the progress of migration.

the approach has been to use compat senders during the transition, I think that makes the most sense here as well. the code here will become easier to grasp if it is fully converted

…stry

jjbayer

Looks good to me in general, just some questions for my own understanding.

relay-server/src/actors/health_check.rs

relay-server/src/actors/project.rs

relay-server/src/actors/health_check.rs

relay-system/src/compat.rs

jjbayer · 2022-10-06T06:54:26Z

relay-metrics/src/aggregation.rs

+    pub partition_key: Option<u64>,
+    /// The buckets to be flushed.
+    pub buckets: Vec<Bucket>,
+}


It's a good idea to reorder these declarations, but it also makes the diff hard to read. What if we keep them where they were, and do the reordering in a separate PR that does not change anything functionally?

If this is difficult to parse I can move those structs back. I decided to move all in one place so it either to see how the API of this service looks like and one can see all the messages and impls for them in one place.

relay-server/src/actors/project.rs

relay-server/src/service.rs

relay-system/src/compat.rs

relay-system/src/controller.rs

relay-metrics/src/aggregation.rs

…stems

…r now

flub · 2022-10-10T10:39:58Z

relay-metrics/benches/aggregator.rs

@@ -1,12 +1,11 @@
-use std::collections::BTreeMap;
-use std::fmt;
+use std::{collections::BTreeMap, fmt};


nitpicking, but what makes this stuff gratuitously change like this? is it some tooling? what's wrong with the existing style?

FWIW while it's not explicitly called out in our how to write rust the example suggests the existing style: https://www.notion.so/sentry/HOWTO-Code-Rust-at-Sentry-7b35f165b10b4492bb95ebe1471a9ada#4f8cc9ec744c472ebcc89791400d519c

This is basically VIM setup I did to merge all the imports if they are coming from the same crate. I will revert it to the previous state.

Oh, is this vim-specific or some rustfmt or rust-analyzer setting that I don't know about? How does this work? I'm curious :)

My config for rustfmt contains

imports_granularity = "Crate"

and then in vim I run following when I want to auto format the file:

> rustfmt +nightly <path to file>

In VSCode / rust-analyzer, I use this:

{ "rust-analyzer.imports.granularity.group": "module", "rust-analyzer.imports.prefix": "crate" }

Will probably check that in. Unfortunately, that doesn't create separate a import group for all relay_* crates, but we can consider removing that convention.

Generally, we run with rustfmt defaults and no overrides.

flub · 2022-10-10T11:08:42Z

relay-metrics/src/aggregation.rs

+                            partition_key,
+                            buckets: batch,
+                        };
+                        compat::send_to_recipient(receiver, flush_buckets);


cool, i agree this is the right way to do this :)

flub · 2022-10-10T11:11:24Z

relay-metrics/src/aggregation.rs

+    fn handle_shutdown(&mut self, message: &Option<Shutdown>) {
+        if let Some(message) = message {
+            if message.timeout.is_some() {
+                self.state = AggregatorState::ShuttingDown;


there used to be a log message here, not sure if it's important.

We already have another message when the aggregator will stop, I don't see the reason to add more logs here.

relay-metrics/src/aggregation.rs

jan-auer

Looks good, thank you!

In tests there are still some broken indentation fixes, if you look at the diff including whitespace you will see them. It would be great to get those reverted before merging this.

flub

same comment on all the indentation stuff - would be great to not pollute the commit history with noise like that

ref(metrics): Migrate Aggregator actor to new tokio runtime

5dc9119

olksdr requested review from jan-auer and jjbayer October 2, 2022 11:22

olksdr self-assigned this Oct 2, 2022

olksdr requested a review from a team October 2, 2022 11:22

olksdr added 2 commits October 2, 2022 13:27

Merge branch 'master' into feat/aggr-to-tokio

1de7596

Add CHANGELOG entry

fce101f

flub reviewed Oct 3, 2022

View reviewed changes

olksdr added 7 commits October 3, 2022 14:47

ref(metrics): small cleanup

6d25dff

Merge branch 'master' into feat/aggr-to-tokio

e4f235e

ref(metrics): address some of the review comments

9726bf8

Merge branch 'master' into feat/aggr-to-tokio

a74f85e

ref(metrics): Add function to get the AggregatorService from the regi…

e04a8b8

…stry

ref(metrics): fix tests and address review comments

f67c635

Merge branch 'master' into feat/aggr-to-tokio

286fc7b

jjbayer reviewed Oct 6, 2022

View reviewed changes

ref(metrics): use function on Registry to get the aggregator

e316a8c

jan-auer requested changes Oct 6, 2022

View reviewed changes

olksdr added 3 commits October 6, 2022 16:07

ref(metrics): add TODOs and use new runtime for aggregator

b6faf59

feat(test): add helper test function to use both new and old tokio sy…

c879274

…stems

ref(metrics): keep the same messages API as in the old style

8879bfb

jan-auer changed the title ~~ref(metrics): Migrate Aggregator actor to new tokio runtime [INGEST-1645]~~ ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] Oct 7, 2022

olksdr added 7 commits October 7, 2022 12:08

ref(metrics): address part2 review comments

963dac7

Merge branch 'master' into feat/aggr-to-tokio

e1ae7fc

ref(actix): fix lint

c58c524

ref(actix): revert to NoResponse in Aggregator interface: use sync fo…

88bca49

…r now

Merge branch 'master' into feat/aggr-to-tokio

d03edce

ref(actix): we must keep aggregator runtime around

f0aa5aa

Fix CHANGELOG, remove unneeded record

f2522cc

olksdr requested a review from jan-auer October 8, 2022 06:37

olksdr requested review from jjbayer and flub October 8, 2022 06:37

ref(actix): update comments

72559a5

flub reviewed Oct 10, 2022

View reviewed changes

ref(actix): add comment and revert the import merge in bences

9c2fb4f

jan-auer approved these changes Oct 10, 2022

View reviewed changes

flub approved these changes Oct 10, 2022

View reviewed changes

olksdr added 2 commits October 11, 2022 07:21

ref(actix): revert formatting in the tests

9489b1f

ref(actix): revert formatting in the last test

06cc3a3

olksdr merged commit c3a1d4c into master Oct 11, 2022

olksdr deleted the feat/aggr-to-tokio branch October 11, 2022 05:51

olksdr mentioned this pull request Oct 17, 2022

Future Proof Relay #1424

Closed

30 tasks

HazAT added this to the Upgrade Tokio in Relay milestone Nov 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] #1508

ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] #1508

olksdr commented Oct 2, 2022

flub left a comment

flub Oct 3, 2022

olksdr Oct 3, 2022

flub Oct 4, 2022

flub Oct 3, 2022

flub Oct 3, 2022

olksdr Oct 3, 2022

flub Oct 4, 2022

jjbayer left a comment

jjbayer Oct 6, 2022

olksdr Oct 6, 2022

flub Oct 10, 2022

olksdr Oct 10, 2022

flub Oct 10, 2022

olksdr Oct 10, 2022

jan-auer Oct 10, 2022 •

edited

Loading

flub Oct 10, 2022

flub Oct 10, 2022

olksdr Oct 10, 2022

jan-auer left a comment

flub left a comment

ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] #1508

ref(actix): Migrate Aggregator actor to new tokio runtime [INGEST-1645] #1508

Conversation

olksdr commented Oct 2, 2022

flub 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

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

jan-auer Oct 10, 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

jan-auer left a comment

Choose a reason for hiding this comment

flub left a comment

Choose a reason for hiding this comment

jan-auer Oct 10, 2022 •

edited

Loading