refactor: Interpret score with a provided method on Score #100

letian · 2025-08-07T15:46:14Z

letian commented

2025-08-07 15:46:14 +00:00

This PR implements the option (2) described below with a bit of "hardcoded" instrumentation. The refactoring to better manage state will come in a future PR because of more decoupling needed (Installable, AlertReceiver, AlertSender, all make use of the InterpretStatus and such).

Context

In the current design, we execute the interpretation of a score as such:

let score = MyScore {};
let result = score.create_interpret().execute(inventory, topology);

The signature being:

trait Score<T: Topology> {
  fn create_interpret(&self) -> Box<dyn Interpret<T>>;
}

trait Interpret<T: Topology> {
  fn execute(&self, inventory: &Inventory, topology: &T) -> Result<InterpretOutcome, InterpretError>;
}

We can do it whenever we want, wherever we want.

Because of this, it is no easy task to control the lifecycle (status) of an Intrepret, without pushing this responsibility back to the developers of scores/interprets. Even if we try to have a "smart" interpret state that handle itself the lifecycle/instrumentation.

So after some consideration, here are the 2 options I explored that allows to keep the flexibility of executing interprets whenever, wherever we want as well as composing scores together.

Possible implementations

1. Introduce an `interpret_score` function

Plays a similar role to Maestro::prepare_topology by "orchestrating" the execution of an Interpret:

fn interpret_score(score: Score, topology: &T) -> Result<InterpretState, InterpretError> {
  let interpret = score.create_interpret();
  let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running }

  let result = interpret.execute(topology);

  match result => {
    Ok(outcome) => match outcome {
      InterpretOutcome::Success { details } => satus.success(details)
      InterpretOutcome::Noop { details } => satus.noop(details)
    }
    Err(err) => status.error(err.message)
  }

  result
}

// example
let score = MyScore {};
let result = interpret_score(score, topology);

Pros:

No need to change existing Score/Interpret implementations

Cons:

Adds an extra function
Doesn't force developers to use it

2. Add `interpret` directly to `Score` (as a provided method)

Similar to proposal (1), the difference is that it's part of the Score trait directly, making it easier to discover and use without needing to remember a separate function:

trait Score {
  fn interpret(&self, topology: &T) -> Result<InterpretState, InterpretError> {
    let interpret = self.create_interpret();
    let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running }

    let result = interpret.execute(topology);

    match result => {
      Ok(outcome) => match outcome {
        InterpretOutcome::Success { details } => satus.success(details)
        InterpretOutcome::Noop { details } => satus.noop(details)
      }
      Err(err) => status.error(err.message)
    }

    result
  }

  fn create_interpret(&self) -> impl Interpret;
}

// example
let score = MyScore {};
let result = score.interpret(topology);

Pros:

No need to change existing Score/Interpret implementations
Easier and more intuitive for developers to discover and use
Since it's part of Score, it feels less like an anemic domain model

Cons:

Still doesn't force developers to use it, but makes the alternative less attractive

This PR implements the option (2) described below with a bit of "hardcoded" instrumentation. The refactoring to better manage state will come in a future PR because of more decoupling needed (`Installable`, `AlertReceiver`, `AlertSender`, all make use of the `InterpretStatus` and such). ## Context In the current design, we execute the interpretation of a score as such: ```rs let score = MyScore {}; let result = score.create_interpret().execute(inventory, topology); ``` The signature being: ```rs trait Score<T: Topology> { fn create_interpret(&self) -> Box<dyn Interpret<T>>; } trait Interpret<T: Topology> { fn execute(&self, inventory: &Inventory, topology: &T) -> Result<InterpretOutcome, InterpretError>; } ``` We can do it whenever we want, wherever we want. Because of this, it is no easy task to control the lifecycle (status) of an Intrepret, without pushing this responsibility back to the developers of scores/interprets. Even if we try to have a "smart" interpret state that handle itself the lifecycle/instrumentation. So after some consideration, here are the 2 options I explored that allows to keep the flexibility of executing interprets whenever, wherever we want as well as composing scores together. ## Possible implementations ### 1. Introduce an `interpret_score` function Plays a similar role to `Maestro::prepare_topology` by "orchestrating" the execution of an `Interpret`: ```rs fn interpret_score(score: Score, topology: &T) -> Result<InterpretState, InterpretError> { let interpret = score.create_interpret(); let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running } let result = interpret.execute(topology); match result => { Ok(outcome) => match outcome { InterpretOutcome::Success { details } => satus.success(details) InterpretOutcome::Noop { details } => satus.noop(details) } Err(err) => status.error(err.message) } result } // example let score = MyScore {}; let result = interpret_score(score, topology); ``` **Pros:** * No need to change existing `Score`/`Interpret` implementations **Cons:** * Adds an extra function * Doesn't force developers to use it ----- ### 2. Add `interpret` directly to `Score` (as a provided method) Similar to proposal (1), the difference is that it's part of the `Score` trait directly, making it easier to discover and use without needing to remember a separate function: ```rs trait Score { fn interpret(&self, topology: &T) -> Result<InterpretState, InterpretError> { let interpret = self.create_interpret(); let mut status = InterpretStatus::execute(topology.name(), score.name(), interpret.name()); // automatically instruments an InterpretStateChanged { status: Running } let result = interpret.execute(topology); match result => { Ok(outcome) => match outcome { InterpretOutcome::Success { details } => satus.success(details) InterpretOutcome::Noop { details } => satus.noop(details) } Err(err) => status.error(err.message) } result } fn create_interpret(&self) -> impl Interpret; } // example let score = MyScore {}; let result = score.interpret(topology); ``` **Pros:** * No need to change existing `Score`/`Interpret` implementations * Easier and more intuitive for developers to discover and use * Since it's part of `Score`, it feels less like an anemic domain model **Cons:** * Still doesn't force developers to use it, but makes the alternative less attractive

letian added 1 commit 2025-08-07 15:46:17 +00:00

refactor: Interpret score with a provided method on Score

Run Check Script / check (pull_request) Has been cancelled

Details

2c8f45c44e

letian added 1 commit 2025-08-07 16:47:48 +00:00

instrument interpret state changes (wip)

Run Check Script / check (pull_request) Has been cancelled

Details

9db475849c

johnride commented

2025-08-08 16:28:46 +00:00

I 100% agree with the pros as required elements of the design :

No need to change existing Score/Interpret implementations
Easier and more intuitive for developers to discover and use
Since it's part of Score, it feels less like an anemic domain model

However, I think the implementation can be better. Adding orchestration-related logic to the Score feels wrong. I'll try something in a playground when I have an hour to spare.

I 100% agree with the pros as required elements of the design : > No need to change existing Score/Interpret implementations > Easier and more intuitive for developers to discover and use > Since it's part of Score, it feels less like an anemic domain model However, I think the implementation can be better. Adding orchestration-related logic to the Score feels wrong. I'll try something in a playground when I have an hour to spare.

👀 1

letian changed target branch from remove-interpret-status-from-topology to master

2025-08-09 22:52:29 +00:00

letian added 1 commit 2025-08-09 22:53:10 +00:00

Merge branch 'master' into interpret-score

Run Check Script / check (pull_request) Has been cancelled

Details

ff011ce1d4

letian merged commit 29a261575b into master

2025-08-09 22:56:26 +00:00

letian deleted branch interpret-score

2025-08-09 22:56:27 +00:00