Add metrics to SimpleScheduler and Worker

[allada]

Adds metrics for running tasks and such to prometheus.

---

This change is 

[allada]

+@aaronmondal do you want to take a swing at reviewing this one? Might be the biggest one of the batch.

Reviewable status: 0 of 10 files reviewed, all discussions resolved (waiting on @aaronmondal)

[allada]

cc: @chrisstaite-menlo, this should give you the metrics you were wanting.

[aaronmondal]

Only got through the first half. Will get back to you on the rest in a bit.

Reviewed 6 of 10 files at r1, all commit messages.
Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @allada)

---

cas/scheduler/default_scheduler_factory.rs line 102 at r1 (raw file):

            // has already been visited, not just the trait. `Any` could be used, but that'd require some rework
            // of all the schedulers. This is the most simple way to do it. Rust's uintptr_t is usize.
            let action_scheduler_uintptr: usize = Arc::as_ptr(action_scheduler) as *const () as usize;

I get why it's done this way but we should probably track this in an issue if there isn't one already.

---

cas/scheduler/simple_scheduler.rs line 237 at r1 (raw file):

        // Check to see if the action is running, if it is and cacheable, merge the actions.
        if let Some(running_action) = self.active_actions.get_mut(&action_info) {
            self.metrics.add_action_joined_running_action.inc();

Nit: I think it would be nice to name this increment. I wasn't sure whether this was something like "include" and had to look up the implementation 😅

---

cas/scheduler/simple_scheduler.rs line 686 at r1 (raw file):

    }

    fn get_inner_lock(&self) -> MutexGuard<'_, SimpleSchedulerImpl> {

I'll get back to you on this one and the wrapping calls. I need to think about those for a bit.

---

cas/scheduler/simple_scheduler.rs line 832 at r1 (raw file):

}

impl MetricsComponent for SimpleScheduler {

Since this code is so "uniform" would it make sense to move all those MetricsComponent impls into a separate file? I like the way it's done in general a lot though.

[allada]

Reviewable status: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @aaronmondal)

---

cas/scheduler/default_scheduler_factory.rs line 102 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I get why it's done this way but we should probably track this in an issue if there isn't one already.

It's not really an issue. I'm not aware of a way to do this without using the raw pointers because they are two completely different interfaces, but might be the same object.

---

cas/scheduler/simple_scheduler.rs line 237 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Nit: I think it would be nice to name this increment. I wasn't sure whether this was something like "include" and had to look up the implementation 😅

You could make the argument .inc() could mean .include(). I thought about .measure(), but i liked how short this function is to reduce the chance it'd get put on 2 lines.

---

cas/scheduler/simple_scheduler.rs line 686 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I'll get back to you on this one and the wrapping calls. I need to think about those for a bit.

FWIW, 99% of the time those wrappers should get inlined.

---

cas/scheduler/simple_scheduler.rs line 832 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

Since this code is so "uniform" would it make sense to move all those MetricsComponent impls into a separate file? I like the way it's done in general a lot though.

I thought about it, but then I realized that because the code is so uniform it is easy for people to realize they are in metrics publication code and scroll back to where they want.

I like the idea of moving it to a new file, but I also don't like the idea of making people question where the metrics for this file are. This file is touched a lot, so we will see if the developer experience sucks because of this. I personally use cold-folding IDEs, so it is easy for me to ignore large sections of code.

[chrisstaite-menlo]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronmondal and @allada)

---

cas/scheduler/default_scheduler_factory.rs line 105 at r1 (raw file):

            if !visited_schedulers.contains(&action_scheduler_uintptr) {
                visited_schedulers.insert(action_scheduler_uintptr);
                action_scheduler.clone().register_metrics(scheduler_metrics);

You could just call register_metrics on the scheduler when it's constructed above instead of all of this... That wouldn't require a map or anything.

[allada]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @aaronmondal and @chrisstaite-menlo)

---

cas/scheduler/default_scheduler_factory.rs line 105 at r1 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

You could just call register_metrics on the scheduler when it's constructed above instead of all of this... That wouldn't require a map or anything.

Yeah, I did consider that. The major reason I liked doing it this way is because it is more standard to how large projects manage it. Because there's a decent chance in the future we may make unregister_metrics() to allow temporary attachment of metrics for debugging. It's more about long-term flexibility.

[chrisstaite-menlo]

cas/scheduler/default_scheduler_factory.rs line 105 at r1 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Yeah, I did consider that. The major reason I liked doing it this way is because it is more standard to how large projects manage it. Because there's a decent chance in the future we may make unregister_metrics() to allow temporary attachment of metrics for debugging. It's more about long-term flexibility.

Is it worth complexity now for possible future expansion? I don't mind either way, and it's only during start-up, so it's not like it's a major issue.

[allada]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aaronmondal)

---

cas/scheduler/default_scheduler_factory.rs line 105 at r1 (raw file):

Previously, chrisstaite-menlo (Chris Staite) wrote…

Is it worth complexity now for possible future expansion? I don't mind either way, and it's only during start-up, so it's not like it's a major issue.

Yeah I'd rather keep it this way, as it feels like this is how most past projects I've worked on do it. I just don't feel like this is one of those cases that it is wroth questioning "why everyone does it this way", when it's just a simple callback vs constructor argument.

[chrisstaite-menlo]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @aaronmondal)

[allada]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aaronmondal)