Make `Trigger` trait generic over `RuntimeFactors` #2790

rylev · 2024-08-30T11:53:41Z

For the Spin CLI this change should be a no-op, but it does open up lots of possibilities for future changes including custom runtimes that benefit from all generic functionality Spin has (e.g., logging, telemetry, etc.) without having to opt into a specific wit world all guests must target.

The main change is that Trigger is now generic the RuntimeFactors type. This means that the set of Factors a Spin runtime can support is no longer hard coded to the TriggerFactors type (which contains implementations for the fermyon:spin/imports world). The decision of which RuntimeFactors type to use is pushed all the way to the edge of the system. Now the spin-cli crate is where TriggerFactors is plugged in. In the future, we may add configuration knobs to tweak which RuntimeFactors type gets plugged in allowing users of the Spin runtime to decide what worlds they wish to support.

The HttpTrigger and RedisTrigger are now also generic over RuntimeFactors meaning that they should be easily pluggable into custom runtimes. Making triggers generic over which RuntimeFactors they support should be easy, but it also allows hardcoding that decision should the trigger author wish to do so.

The key to all of this working is the RuntimeFactorsBuilder which makes constructing and configuring a RuntimeFactors type generic. This includes the ability for implementors of this trait to specify custom CLI arguments that are just used for configuring the RuntimeFactors type.

In the generic trigger code, there are a few places where a RuntimeFactors type is expected to have a certain Factor in its collection. This has been changed to a dynamic query that allows generic code to do something if a RuntimeFactors contains a certain Factor. This is accomplished through the new HasInstanceBuilder trait.

rylev · 2024-08-30T11:54:32Z

crates/runtime-config/src/lib.rs

@@ -19,7 +19,9 @@ use spin_factors::runtime_config::toml::GetTomlValue as _;
 use spin_factors::{


This file can largely be ignored for review should you wish to as it's mostly just surface level refactoring.

rylev · 2024-08-30T11:55:43Z

crates/trigger/src/cli.rs

        let mut core_engine_builder = {
            self.trigger.update_core_config(&mut self.engine_config)?;

            spin_core::Engine::builder(&self.engine_config)?
        };
        self.trigger.add_to_linker(core_engine_builder.linker())?;

-        let runtime_config_path = options.runtime_config_file;


All of this logic has been moved to the RuntimeFactorsBuilder implementation in spin-cli.

rylev · 2024-08-30T11:56:54Z

crates/trigger/src/cli/initial_kv_setter.rs

@@ -0,0 +1,42 @@
+use anyhow::Context as _;


This file might seem to be an unnecessary change, but it was the easiest way to continue to support setting key-value pairs on start up. I imagine even if we don't accept this PR, we'll want to implement this change.

rylev · 2024-08-30T11:57:18Z

crates/trigger/src/cli/summary.rs

-
-use crate::factors::TriggerFactors;
-
-pub fn summarize_runtime_config<T>(


All of this has been moved to the spin-cli crate.

examples/spin-timer/Cargo.toml

radu-matei

Conceptually, I am 100% in favor of this change — it is a huge step in the direction of allowing embedders to assemble their own runtime that makes zero assumptions about the world they are targeting (and more specifically, making it trivial to build a trigger that does not implicitly assume the Spin world).

Thank you for the work that went into this!
(I plan to review in more detail and test this soon, but wanted to share my initial thoughts early on)

lann

initial pass

crates/factors/src/runtime_factors.rs

crates/trigger-http/src/server.rs

crates/trigger-http/src/wagi.rs

lann · 2024-08-30T12:52:54Z

crates/trigger-redis/src/lib.rs

        let app_variables = trigger_app
            .configured_app()
            .app_state::<VariablesFactor>()
            .context("RedisTrigger depends on VariablesFactor")?;

        let app = trigger_app.app();
+        let trigger_type = <Self as Trigger<F>>::TYPE;


Could probably avoid this awkwardness by adding a Trigger::trigger_type() -> &'static str method. 🤷

crates/runtime-factors/src/lib.rs

lann · 2024-08-30T12:57:21Z

crates/runtime-factors/Cargo.toml

@@ -0,0 +1,41 @@
+[package]
+name = "spin-runtime-factors"


Probably need a different name here given that RuntimeFactors is already a thing...

I fully agree. I'd love to have a succienct way to differentiate between generic "any Spin runtime code" vs. code specific to a specific runtime. One could be tempted to call this spin-cli-runtime-factors, but I think that's almost too specific.

I've been thinking that "the spin up runtime" might be a good way to refer to this most-opinionated layer of the runtime. Maybe spin-up-runtime-factors here?

crates/trigger/src/cli.rs

lann · 2024-09-03T13:02:46Z

crates/runtime-config/src/lib.rs

@@ -46,82 +49,85 @@ pub struct ResolvedRuntimeConfig<T> {
    pub toml: toml::Table,
 }

+impl<T> ResolvedRuntimeConfig<T> {
+    pub fn summarize(&self, runtime_config_path: Option<&Path>) {


Moving this code here seems a bit odd. Is this crate intended to be tightly coupled to the Spin CLI?

If it stays here, maybe rename to something like print_summary to make its purpose more clear?

Yea I agree - though the inlining it is also not great. If someone wants to create an alternate RuntimeFactorsBuilder and still print out this summary (which I would expect since they're already going to get all other logging for free), they'll have to copy this code from the spin-runtime-factors crate.

Thinking about this more - I think as the crate stands today, this is the best place for this. This has a high chance to change though as we start tweaking things.

lann · 2024-09-03T13:06:02Z

crates/runtime-factors/Cargo.toml

@@ -0,0 +1,41 @@
+[package]
+name = "spin-runtime-factors"


I've been thinking that "the spin up runtime" might be a good way to refer to this most-opinionated layer of the runtime. Maybe spin-up-runtime-factors here?

crates/trigger/src/cli.rs

Signed-off-by: Ryan Levick <[email protected]>

Signed-off-by: Lann Martin <[email protected]>

Signed-off-by: Ryan Levick <[email protected]>

rylev requested a review from lann August 30, 2024 11:53

rylev commented Aug 30, 2024

View reviewed changes

examples/spin-timer/Cargo.toml Outdated Show resolved Hide resolved

radu-matei reviewed Aug 30, 2024

View reviewed changes

lann reviewed Aug 30, 2024

View reviewed changes

rylev force-pushed the generic-factors2 branch 3 times, most recently from 9fef2b1 to c661f97 Compare September 3, 2024 09:22

lann reviewed Sep 3, 2024

View reviewed changes

rylev force-pushed the generic-factors2 branch from c661f97 to de1172b Compare September 3, 2024 13:49

lann approved these changes Sep 3, 2024

View reviewed changes

rylev and others added 13 commits September 4, 2024 11:07

Working generic factors

30962b7

Signed-off-by: Ryan Levick <[email protected]>

Separate common trigger options from Factors specific ones.

cc81c12

Signed-off-by: Ryan Levick <[email protected]>

Move where user defined paths are defined

98027a2

Signed-off-by: Ryan Levick <[email protected]>

Feed runtime all the way through

4753301

Signed-off-by: Ryan Levick <[email protected]>

Allow dynamic querying of instance builders

bfd3680

Signed-off-by: Ryan Levick <[email protected]>

Move Spin CLI runtime factors to their own crate

8b7f497

Signed-off-by: Ryan Levick <[email protected]>

Reimplement initial kv handling

edac482

Signed-off-by: Ryan Levick <[email protected]>

Fix rebasing issues

3ce559f

Signed-off-by: Ryan Levick <[email protected]>

Rearrange so that spin-cli isn't a dependency

45491d2

Signed-off-by: Ryan Levick <[email protected]>

PR feedback

1d0c79e

Signed-off-by: Ryan Levick <[email protected]>

factors: Remove InstanceFactors

7dcbc4e

Signed-off-by: Lann Martin <[email protected]>

Move to build mod

7149cf0

Signed-off-by: Ryan Levick <[email protected]>

Move summarize function to runtime config crate

786e803

Signed-off-by: Ryan Levick <[email protected]>

rylev force-pushed the generic-factors2 branch from de1172b to 786e803 Compare September 4, 2024 09:25

rylev enabled auto-merge September 4, 2024 09:29

rylev merged commit 3e62d2e into main Sep 4, 2024
17 checks passed

rylev deleted the generic-factors2 branch September 4, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Trigger` trait generic over `RuntimeFactors` #2790

Make `Trigger` trait generic over `RuntimeFactors` #2790

rylev commented Aug 30, 2024 •

edited

Loading

rylev Aug 30, 2024

rylev Aug 30, 2024

rylev Aug 30, 2024

rylev Aug 30, 2024

radu-matei left a comment

lann left a comment

lann Aug 30, 2024

lann Aug 30, 2024

rylev Aug 30, 2024

lann Sep 3, 2024 •

edited

Loading

lann Sep 3, 2024

rylev Sep 3, 2024

rylev Sep 3, 2024

lann Sep 3, 2024 •

edited

Loading

		@@ -19,7 +19,9 @@ use spin_factors::runtime_config::toml::GetTomlValue as _;
		use spin_factors::{


		use crate::factors::TriggerFactors;

		pub fn summarize_runtime_config<T>(

Make Trigger trait generic over RuntimeFactors #2790

Make Trigger trait generic over RuntimeFactors #2790

Conversation

rylev commented Aug 30, 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

radu-matei left a comment

Choose a reason for hiding this comment

lann 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

lann Sep 3, 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

lann Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Make `Trigger` trait generic over `RuntimeFactors` #2790

Make `Trigger` trait generic over `RuntimeFactors` #2790

rylev commented Aug 30, 2024 •

edited

Loading

lann Sep 3, 2024 •

edited

Loading

lann Sep 3, 2024 •

edited

Loading