fix(cli): reduce noise & better track progress within Harmony #91

letian · 2025-07-27T21:42:59Z

letian commented

2025-07-27 21:42:59 +00:00

Introduce a way to instrument what happens within Harmony and around Harmony (e.g. in the CLI or in Composer).

The goal is to provide visual feedback to the end users and inform them of the progress of their tasks (e.g. deployment) as clearly as possible. It is important to also let them know of the outcome of their tasks (what was created, where to access stuff, etc.).

Changes

Instrumentation architecture

Extensibility and ease of use is key here, while preserving type safety as much as possible.

The proposed API is quite simple:

// Emit an event
instrumentation::instrument(
    HarmonyEvent::TopologyPrepared {
        topology: "k8s-anywhere",
        outcome: Outcome::success("yay")
    }
);

// Consume events
instrumentation::subscribe("Harmony CLI Logger", async |event| {
    match event {
        HarmonyEvent::TopologyPrepared { name, outcome } => todo!(),
    }
});

Current limitations

this API is not very extensible, but it could be easily changed to allow end users to define custom events in addition to Harmony core events
we use a tokio broadcast channel behind the scene so only in process communication can happen, but it could be easily changed to a more flexible communication mechanism as implementation details are hidden

`harmony_composer` VS `harmony_cli`

As Harmony Composer launches commands from Harmony (CLI), they both live in different processes. And because of this, we cannot easily make all the logging happens in one place (Harmony Composer) and get rid of Harmony CLI. At least not without introducing additional complexity such as communication through a server, unix socket, etc.

So for the time being, it was decided to preserve both harmony_composer and harmony_cli and let them independently log their stuff and handle their own responsibilities:

harmony_composer: takes care only of setting up & packaging a project, delegates everything else to harmony_cli
harmony_cli: takes care of configuring & running Harmony

Logging & prompts

indicatif is used to create progress bars and track progress within Harmony, Harmony CLI, and Harmony Composer
inquire is preserved, but was removed from harmony (core) as UI concerns shouldn't go that deep
- note: for now the only prompt we had was simply deleted, we'll have to find a better way to prompt stuff in the future

Todos

Update/Create ADRs
Continue instrumentation for missing branches
Allow instrumentation to emit and subscribe to custom events

Introduce a way to instrument what happens within Harmony and around Harmony (e.g. in the CLI or in Composer). The goal is to provide visual feedback to the end users and inform them of the progress of their tasks (e.g. deployment) as clearly as possible. It is important to also let them know of the outcome of their tasks (what was created, where to access stuff, etc.). <img src="https://media.discordapp.net/attachments/1295353830300713062/1400289618636574741/demo.gif?ex=688c18d5&is=688ac755&hm=2c70884aacb08f7bd15cbb65a7562a174846906718aa15294bbb238e64febbce&=" /> ## Changes ### Instrumentation architecture Extensibility and ease of use is key here, while preserving type safety as much as possible. The proposed API is quite simple: ```rs // Emit an event instrumentation::instrument( HarmonyEvent::TopologyPrepared { topology: "k8s-anywhere", outcome: Outcome::success("yay") } ); // Consume events instrumentation::subscribe("Harmony CLI Logger", async |event| { match event { HarmonyEvent::TopologyPrepared { name, outcome } => todo!(), } }); ``` #### Current limitations * this API is not very extensible, but it could be easily changed to allow end users to define custom events in addition to Harmony core events * we use a tokio broadcast channel behind the scene so only in process communication can happen, but it could be easily changed to a more flexible communication mechanism as implementation details are hidden ### `harmony_composer` VS `harmony_cli` As Harmony Composer launches commands from Harmony (CLI), they both live in different processes. And because of this, we cannot easily make all the logging happens in one place (Harmony Composer) and get rid of Harmony CLI. At least not without introducing additional complexity such as communication through a server, unix socket, etc. So for the time being, it was decided to preserve both `harmony_composer` and `harmony_cli` and let them independently log their stuff and handle their own responsibilities: * `harmony_composer`: takes care only of setting up & packaging a project, delegates everything else to `harmony_cli` * `harmony_cli`: takes care of configuring & running Harmony ### Logging & prompts * [indicatif](https://github.com/console-rs/indicatif) is used to create progress bars and track progress within Harmony, Harmony CLI, and Harmony Composer * [inquire](https://github.com/mikaelmello/inquire) is preserved, but was removed from `harmony` (core) as UI concerns shouldn't go that deep * note: for now the only prompt we had was simply deleted, we'll have to find a better way to prompt stuff in the future ## Todos * [ ] Update/Create ADRs * [ ] Continue instrumentation for missing branches * [ ] Allow instrumentation to emit and subscribe to custom events

letian added 1 commit 2025-07-27 21:43:04 +00:00

fix(cli): reduce noise & better track progress within Harmony

Run Check Script / check (pull_request) Successful in -35s

Details

6f7e1640c1

letian added 1 commit 2025-07-28 00:52:40 +00:00

split instrumentation in 2 different places: harmony domain (for domain observability) & harmoy composer (for build/commands observability)

Run Check Script / check (pull_request) Successful in -34s

Details

8fae9cf8c8

letian reviewed 2025-07-28 02:05:55 +00:00

examples/rust/src/main.rs Outdated

						
				@ -13,3 +14,4 @@

				#[tokio::main]

				async fn main() {

				    env_logger::init();

				    let cli_logger_handle = tokio::spawn(cli_logger::init());

letian commented

2025-07-28 02:05:55 +00:00

Ideally, this shouldn't have to be handled by the end user. But in this case, because Maestro::initialize prepares the Topology (which might execute Scores), we have to register the cli_logger early.

All of this could be improved.

Ideally, this shouldn't have to be handled by the end user. But in this case, because `Maestro::initialize` prepares the `Topology` (which might execute `Scores`), we have to register the `cli_logger` early. All of this could be improved.

letian commented

2025-07-31 13:32:03 +00:00

@johnride About this, could we actually reverse things a little bit?

As of now, when we initialize the maestro, it tries to prepare the Topology right away. But because of this, when later on we want to interpret our scores, we have to check whether the topology was actually initialized. Which might end up in a weird situation when it's not the case.

So moving the initialization inside the Maestro::interpret will at the same time prevent this odd temporal dependency and gives us the ability to hide the cli_logger::init (because we can move it inside harmony_cli::init).

Another side effect is that we would be able to remove the Maestro::new_without_initialization and the Maestro::initialize and keep only a bare Maestro::new, and thus solve our naming issues 😅

It's also a bit related to what we started talking together regarding the CLI pre-filtering the scores and asking the Maestro to interpret them one by one.

@johnride About this, could we actually reverse things a little bit? As of now, when we initialize the maestro, it tries to prepare the Topology right away. But because of this, when later on we want to interpret our scores, we have to check whether the topology was actually initialized. Which might end up in a weird situation when it's not the case. So moving the initialization inside the `Maestro::interpret` will at the same time prevent this odd temporal dependency and gives us the ability to hide the `cli_logger::init` (because we can move it inside `harmony_cli::init`). Another side effect is that we would be able to remove the `Maestro::new_without_initialization` and the `Maestro::initialize` and keep only a bare `Maestro::new`, and thus solve our naming issues 😅 It's also a bit related to what we started talking together regarding the CLI pre-filtering the scores and asking the Maestro to interpret them one by one.

johnride commented

2025-07-31 15:59:32 +00:00

There me be another side effect to initializing the topology immediately : the scores we are launching from various places in the code.

They might be relying on the Topology being ready, and they don't use the maestro to interpret the score.

The conceptual idea is that we want to fail early on anything that can be checked when we launch the app. For example, checking that the topology is as expected at compile time. Let's say we compile an AWS Topology but we're not running in AWS or there is no AWS credential available, then we want to fail immediately, or at least realize this immediately and be able to react accordingly.

I agree the naming is smelly but apart from new_without_initialize which is a utility function that should rarely be used, it feels correct to me to instanciate the maestro with initialize so it explicitely performs its checks early on.

There me be another side effect to initializing the topology immediately : the scores we are launching from various places in the code. They might be relying on the Topology being ready, and they don't use the maestro to interpret the score. The conceptual idea is that we want to fail early on anything that can be checked when we launch the app. For example, checking that the topology is as expected at compile time. Let's say we compile an AWS Topology but we're not running in AWS or there is no AWS credential available, then we want to fail immediately, or at least realize this immediately and be able to react accordingly. I agree the naming is smelly but apart from `new_without_initialize` which is a utility function that should rarely be used, it feels correct to me to instanciate the maestro with initialize so it explicitely performs its checks early on.

letian reviewed 2025-07-28 02:07:12 +00:00

harmony_cli/src/lib.rs

						
				@ -4,6 +4,9 @@ use harmony;

				use harmony::{score::Score, topology::Topology};

				use inquire::Confirm;

				pub mod cli_logger; // FIXME: Don't make me pub

letian commented

2025-07-28 02:07:12 +00:00

See the PR comment above (in examples/rust/main.rs) for more info

See the PR comment above (in `examples/rust/main.rs`) for more info

letian reviewed 2025-07-28 02:09:28 +00:00

harmony_composer/src/instrumentation.rs

						
				@ -0,0 +14,4 @@

				    Shutdown,

				}

				static HARMONY_COMPOSER_EVENT_BUS: Lazy<broadcast::Sender<HarmonyComposerEvent>> =

letian commented

2025-07-28 02:09:28 +00:00

I'm not a big fan of "duplicating" all the stuff below. It is for different concerns than the ones defined in harmony_cli, but it follows the same pattern.

I'm not a big fan of "duplicating" all the stuff below. It is for different concerns than the ones defined in `harmony_cli`, but it follows the same pattern.

letian reviewed 2025-07-28 02:10:19 +00:00

harmony_composer/src/main.rs Outdated

						
				@ -138,2 +155,4 @@

				        None => todo!("run interactively, ask for info on CLI"),

				    }

				    instrumentation::instrument(HarmonyComposerEvent::Shutdown);

letian commented

2025-07-28 02:10:19 +00:00

Some instrumentations are still missing, but this PR is mostly to get early feedback on the approach.

letian reviewed 2025-07-28 02:12:07 +00:00

harmony_composer/src/main.rs Outdated

						
				@ -186,6 +220,12 @@ async fn compile_cargo(platform: String, harmony_location: String) -> PathBuf {

				        .exec()

				        .unwrap();

				    let stderr = if log_enabled!(log::Level::Debug) {

letian commented

2025-07-28 02:12:06 +00:00

cargo prints warnings and other stuff in stderr, meaning it can get really noisy. So we'll "silence" those warnings unless we're asking for debug logs (RUST_LOG=debug)

`cargo` prints warnings and other stuff in `stderr`, meaning it can get really noisy. So we'll "silence" those warnings unless we're asking for debug logs (`RUST_LOG=debug`)

letian reviewed 2025-07-29 12:45:19 +00:00

letian added 1 commit 2025-07-30 16:22:19 +00:00

ensure event handlers are properly subscribed on init, extract duplicated progress functions, cleanup duplication

Run Check Script / check (pull_request) Successful in -31s

Details

ff7801a7c1

letian added 1 commit 2025-07-31 01:34:47 +00:00

add event to track progress of interprets, change a bunch of info! to debug!

Run Check Script / check (pull_request) Successful in -34s

Details

49f1e56599

letian reviewed 2025-07-31 01:39:31 +00:00

harmony/src/domain/instrumentation.rs

						
				@ -0,0 +35,4 @@

				pub fn instrument(event: HarmonyEvent) -> Result<(), &'static str> {

				    match HARMONY_EVENT_BUS.send(event) {

				        Ok(_) => Ok(()),

				        Err(_) => Err("send error: no subscribers"),

letian commented

2025-07-31 01:39:31 +00:00

some kind of retry mechanism could be useful here to make sure no events are lost

👍 1

letian added 1 commit 2025-07-31 01:42:19 +00:00

remove unused inquire dependency for Harmony

Run Check Script / check (pull_request) Successful in -36s

Details

68fde23f2c

letian added 1 commit 2025-07-31 12:05:22 +00:00

add k3d todo

Run Check Script / check (pull_request) Successful in -35s

Details

507556969a

letian changed title from ~~WIP: fix(cli): reduce noise & better track progress within Harmony~~ to fix(cli): reduce noise & better track progress within Harmony

2025-07-31 13:08:29 +00:00

letian changed target branch from simply-k3d-installation to master

2025-07-31 13:23:00 +00:00

letian added 1 commit 2025-07-31 13:24:21 +00:00

rename some event attributes

Run Check Script / check (pull_request) Successful in -38s

Details

6b36b1c7e9

letian added 1 commit 2025-07-31 13:24:44 +00:00

Merge branch 'master' into better-cli

Run Check Script / check (pull_request) Successful in -44s

Details

39208c5e86

johnride approved these changes 2025-07-31 16:09:28 +00:00

johnride left a comment

Overall I have a feeling like this is too specific on each tiny little even type, it might make maintenance and extensibility quite a burden.

It's OK for now, let's go forward with this until we see a pattern emerge that will allow simplification, if ever.

Overall I have a feeling like this is too specific on each tiny little even type, it might make maintenance and extensibility quite a burden. It's OK for now, let's go forward with this until we see a pattern emerge that will allow simplification, if ever.

letian merged commit 06aab1f57f into master

2025-07-31 19:35:36 +00:00

letian referenced this issue from a commit

2025-07-31 19:35:38 +00:00

fix(cli): reduce noise & better track progress within Harmony (#91)

Sign in to join this conversation.

No reviewers