From 367d5a3a0d672034a57d4626fd370eddc876f4e2 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 13:28:02 +0100 Subject: [PATCH 01/11] chore(instrumentation): Reduce noise from instrumented functions --- rust/stackable-cockpit/src/engine/docker/mod.rs | 2 +- rust/stackable-cockpit/src/engine/kind/mod.rs | 6 +++--- .../stackable-cockpit/src/engine/minikube/mod.rs | 6 +++--- rust/stackable-cockpit/src/helm.rs | 11 ++++++----- rust/stackable-cockpit/src/platform/demo/spec.rs | 11 ++++++++++- rust/stackable-cockpit/src/platform/manifests.rs | 13 +++++-------- .../src/platform/operator/mod.rs | 13 ++++++++++--- .../src/platform/release/spec.rs | 11 +++++++++-- .../stackable-cockpit/src/platform/stack/spec.rs | 12 +++++++++--- rust/stackable-cockpit/src/utils/check.rs | 4 ++-- rust/stackablectl/src/cli/mod.rs | 6 ++---- rust/stackablectl/src/cmds/demo.rs | 12 ++++++++---- rust/stackablectl/src/cmds/operator.rs | 16 ++++++++-------- rust/stackablectl/src/cmds/stack.rs | 4 ++-- rust/stackablectl/src/cmds/stacklet.rs | 4 ++-- 15 files changed, 80 insertions(+), 51 deletions(-) diff --git a/rust/stackable-cockpit/src/engine/docker/mod.rs b/rust/stackable-cockpit/src/engine/docker/mod.rs index 1a7e515e..697b0797 100644 --- a/rust/stackable-cockpit/src/engine/docker/mod.rs +++ b/rust/stackable-cockpit/src/engine/docker/mod.rs @@ -19,7 +19,7 @@ pub enum Error { } /// Checks if Docker is running on the system -#[instrument] +#[instrument(skip_all)] pub async fn check_if_docker_is_running() -> Result<()> { debug!("Checking if Docker is running"); diff --git a/rust/stackable-cockpit/src/engine/kind/mod.rs b/rust/stackable-cockpit/src/engine/kind/mod.rs index 96a159c5..85b8843f 100644 --- a/rust/stackable-cockpit/src/engine/kind/mod.rs +++ b/rust/stackable-cockpit/src/engine/kind/mod.rs @@ -64,7 +64,7 @@ impl Cluster { } /// Create a new local cluster by calling the kind binary. - #[instrument] + #[instrument(skip_all)] pub async fn create(&self) -> Result<()> { info!("Creating local cluster using kind"); @@ -109,7 +109,7 @@ impl Cluster { } /// Creates a kind cluster if it doesn't exist already. - #[instrument] + #[instrument(skip_all)] pub async fn create_if_not_exists(&self) -> Result<()> { info!("Creating cluster if it doesn't exist using kind"); @@ -131,7 +131,7 @@ impl Cluster { } /// Check if a kind cluster with the provided name already exists. - #[instrument] + #[instrument(skip_all)] async fn check_if_cluster_exists(cluster_name: &str) -> Result { debug!("Checking if kind cluster exists"); diff --git a/rust/stackable-cockpit/src/engine/minikube/mod.rs b/rust/stackable-cockpit/src/engine/minikube/mod.rs index a8a929f5..dab6e959 100644 --- a/rust/stackable-cockpit/src/engine/minikube/mod.rs +++ b/rust/stackable-cockpit/src/engine/minikube/mod.rs @@ -41,7 +41,7 @@ impl Cluster { } /// Create a new local cluster by calling the Minikube binary - #[instrument] + #[instrument(skip_all)] pub async fn create(&self) -> Result<(), Error> { info!("Creating local cluster using Minikube"); @@ -70,7 +70,7 @@ impl Cluster { } /// Creates a Minikube cluster if it doesn't exist already. - #[instrument] + #[instrument(skip_all)] pub async fn create_if_not_exists(&self) -> Result<(), Error> { info!("Creating cluster if it doesn't exist using Minikube"); @@ -92,7 +92,7 @@ impl Cluster { } /// Check if a kind cluster with the provided name already exists. - #[instrument] + #[instrument(skip_all)] async fn check_if_cluster_exists(cluster_name: &str) -> Result { debug!("Checking if Minikube cluster exists"); diff --git a/rust/stackable-cockpit/src/helm.rs b/rust/stackable-cockpit/src/helm.rs index 25bca566..fd263aad 100644 --- a/rust/stackable-cockpit/src/helm.rs +++ b/rust/stackable-cockpit/src/helm.rs @@ -182,7 +182,7 @@ pub struct ChartVersion<'a> { /// /// This function expects the fully qualified Helm release name. In case of our /// operators this is: `-operator`. -#[instrument] +#[instrument(skip(values_yaml), fields(with_values = values_yaml.is_some()))] pub fn install_release_from_repo_or_registry( release_name: &str, ChartVersion { @@ -239,8 +239,8 @@ pub fn install_release_from_repo_or_registry( let chart_version = chart_version.unwrap_or(HELM_DEFAULT_CHART_VERSION); debug!( - "Installing Helm release {} ({}) from chart {}", - release_name, chart_version, full_chart_name + release_name, + chart_version, full_chart_name, "Installing Helm release" ); install_release( @@ -260,6 +260,7 @@ pub fn install_release_from_repo_or_registry( /// /// This function expects the fully qualified Helm release name. In case of our /// operators this is: `-operator`. +#[instrument(fields(with_values = values_yaml.is_some()))] fn install_release( release_name: &str, chart_name: &str, @@ -388,10 +389,10 @@ pub fn add_repo(repository_name: &str, repository_url: &str) -> Result<(), Error } /// Retrieves the Helm index file from the repository URL. -#[instrument] +#[instrument(skip_all, fields(%repo_url))] pub async fn get_helm_index(repo_url: T) -> Result where - T: AsRef + std::fmt::Debug, + T: AsRef + std::fmt::Display + std::fmt::Debug, { debug!("Get Helm repo index file"); diff --git a/rust/stackable-cockpit/src/platform/demo/spec.rs b/rust/stackable-cockpit/src/platform/demo/spec.rs index 59aaa8d9..af464125 100644 --- a/rust/stackable-cockpit/src/platform/demo/spec.rs +++ b/rust/stackable-cockpit/src/platform/demo/spec.rs @@ -134,6 +134,11 @@ impl DemoSpec { Ok(()) } + #[instrument(skip_all, fields( + stack_name = %self.stack, + operator_namespace = %install_parameters.operator_namespace, + product_namespace = %install_parameters.product_namespace, + ))] pub async fn install( &self, stack_list: StackList, @@ -177,7 +182,11 @@ impl DemoSpec { .await } - #[instrument(skip_all)] + #[instrument(skip_all, fields( + stack_name = %self.stack, + operator_namespace = %install_params.operator_namespace, + product_namespace = %install_params.product_namespace, + ))] async fn prepare_manifests( &self, install_params: DemoInstallParameters, diff --git a/rust/stackable-cockpit/src/platform/manifests.rs b/rust/stackable-cockpit/src/platform/manifests.rs index 86b604f4..b92814c1 100644 --- a/rust/stackable-cockpit/src/platform/manifests.rs +++ b/rust/stackable-cockpit/src/platform/manifests.rs @@ -62,7 +62,7 @@ pub enum Error { pub trait InstallManifestsExt { // TODO (Techassi): This step shouldn't care about templating the manifests nor fetching them from remote - #[instrument(skip_all)] + #[instrument(skip_all, fields(%product_namespace))] #[allow(async_fn_in_trait)] async fn install_manifests( manifests: &[ManifestSpec], @@ -72,12 +72,12 @@ pub trait InstallManifestsExt { client: &Client, transfer_client: &xfer::Client, ) -> Result<(), Error> { - debug!("Installing demo / stack manifests"); + debug!("Installing manifests"); for manifest in manifests { match manifest { ManifestSpec::HelmChart(helm_file) => { - debug!("Installing manifest from Helm chart {}", helm_file); + debug!(helm_file, "Installing manifest from Helm chart"); // Read Helm chart YAML and apply templating let helm_file = helm_file.into_path_or_url().context(ParsePathOrUrlSnafu { @@ -89,10 +89,7 @@ pub trait InstallManifestsExt { .await .context(FileTransferSnafu)?; - info!( - "Installing Helm chart {} ({})", - helm_chart.name, helm_chart.version - ); + info!(helm_chart.name, helm_chart.version, "Installing Helm chart",); // Assumption: that all manifest helm charts refer to repos not registries helm::add_repo(&helm_chart.repo.name, &helm_chart.repo.url).context( @@ -122,7 +119,7 @@ pub trait InstallManifestsExt { })?; } ManifestSpec::PlainYaml(manifest_file) => { - debug!("Installing YAML manifest from {}", manifest_file); + debug!(manifest_file, "Installing YAML manifest"); // Read YAML manifest and apply templating let path_or_url = diff --git a/rust/stackable-cockpit/src/platform/operator/mod.rs b/rust/stackable-cockpit/src/platform/operator/mod.rs index eedcf9f9..556943bc 100644 --- a/rust/stackable-cockpit/src/platform/operator/mod.rs +++ b/rust/stackable-cockpit/src/platform/operator/mod.rs @@ -178,7 +178,14 @@ impl OperatorSpec { } /// Installs the operator using Helm. - #[instrument(skip_all)] + #[instrument(skip_all, fields( + %namespace, + name = %self.name, + // NOTE (@NickLarsenNZ): Option doesn't impl Display, so we need to call + // display for the inner type if it exists. Otherwise we gte the Debug + // impl for the whole Option. + version = self.version.as_ref().map(|v| tracing::field::display(v)), + ))] pub fn install( &self, namespace: &str, @@ -213,10 +220,10 @@ impl OperatorSpec { } /// Uninstalls the operator using Helm. - #[instrument] + #[instrument(skip_all, fields(%namespace))] pub fn uninstall(&self, namespace: T) -> Result<(), helm::Error> where - T: AsRef + std::fmt::Debug, + T: AsRef + std::fmt::Display + std::fmt::Debug, { match helm::uninstall_release(&self.helm_name(), namespace.as_ref(), true) { Ok(status) => { diff --git a/rust/stackable-cockpit/src/platform/release/spec.rs b/rust/stackable-cockpit/src/platform/release/spec.rs index cf4bb8cd..bcdf5ccc 100644 --- a/rust/stackable-cockpit/src/platform/release/spec.rs +++ b/rust/stackable-cockpit/src/platform/release/spec.rs @@ -3,7 +3,7 @@ use indexmap::IndexMap; use serde::{Deserialize, Serialize}; use snafu::{ResultExt, Snafu}; use tokio::task::JoinError; -use tracing::{info, instrument}; +use tracing::{info, instrument, Instrument, Span}; #[cfg(feature = "openapi")] use utoipa::ToSchema; @@ -50,7 +50,7 @@ pub struct ReleaseSpec { impl ReleaseSpec { /// Installs a release by installing individual operators. - #[instrument(skip_all)] + #[instrument(skip_all, fields(%namespace, product.included = tracing::field::Empty, product.excluded = tracing::field::Empty))] pub async fn install( &self, include_products: &[String], @@ -60,6 +60,13 @@ impl ReleaseSpec { ) -> Result<()> { info!("Installing release"); + include_products.iter().for_each(|product| { + Span::current().record("product.included", product); + }); + exclude_products.iter().for_each(|product| { + Span::current().record("product.excluded", product); + }); + let namespace = namespace.to_string(); futures::stream::iter(self.filter_products(include_products, exclude_products)) .map(|(product_name, product)| { diff --git a/rust/stackable-cockpit/src/platform/stack/spec.rs b/rust/stackable-cockpit/src/platform/stack/spec.rs index 2a20ba5d..2fd712e1 100644 --- a/rust/stackable-cockpit/src/platform/stack/spec.rs +++ b/rust/stackable-cockpit/src/platform/stack/spec.rs @@ -151,7 +151,13 @@ impl StackSpec { } // TODO (Techassi): Can we get rid of the release list and just use the release spec instead - #[instrument(skip(self, release_list, client, transfer_client))] + #[instrument(skip_all, fields( + stack_name = %install_parameters.stack_name, + // NOTE (@NickLarsenNZ): Option doesn't impl Display, so we need to call + // display for the inner type if it exists. Otherwise we gte the Debug + // impl for the whole Option. + demo_name = install_parameters.demo_name.as_ref().map(|v| tracing::field::display(v)), + ))] pub async fn install( &self, release_list: release::ReleaseList, @@ -192,12 +198,12 @@ impl StackSpec { .await } - #[instrument(skip(self, release_list))] + #[instrument(skip_all, fields(release = %self.release, %operator_namespace))] pub async fn install_release( &self, release_list: release::ReleaseList, operator_namespace: &str, - product_namespace: &str, + _product_namespace: &str, // TODO (@NickLarsenNZ): remove this field chart_source: &ChartSourceType, ) -> Result<(), Error> { info!("Trying to install release {}", self.release); diff --git a/rust/stackable-cockpit/src/utils/check.rs b/rust/stackable-cockpit/src/utils/check.rs index 8abf551b..bb67806c 100644 --- a/rust/stackable-cockpit/src/utils/check.rs +++ b/rust/stackable-cockpit/src/utils/check.rs @@ -9,7 +9,7 @@ pub fn binary_present>(name: T) -> bool { } /// Returns if ALL binaries in the list are present in the $PATH. -#[instrument] +#[instrument(skip_all)] pub fn binaries_present(names: &[&str]) -> bool { debug!("Checking if required binaries are present on the system"); @@ -24,7 +24,7 @@ pub fn binaries_present(names: &[&str]) -> bool { /// Returns [`None`] if all binaries in the list are present in the $PATH and /// if not, returns [`Some`] containing the name of the missing binary. -#[instrument] +#[instrument(skip_all)] pub fn binaries_present_with_name(names: &[&str]) -> Option { debug!("Checking if required binaries are present on the system"); diff --git a/rust/stackablectl/src/cli/mod.rs b/rust/stackablectl/src/cli/mod.rs index 6ac718b1..15e48053 100644 --- a/rust/stackablectl/src/cli/mod.rs +++ b/rust/stackablectl/src/cli/mod.rs @@ -135,9 +135,8 @@ impl Cli { /// Adds the default (or custom) Helm repository URLs. Internally this calls the Helm SDK written in Go through the /// `go-helm-wrapper`. - #[instrument] pub fn add_helm_repos(&self) -> Result<(), helm::Error> { - debug!("Add Helm repos"); + tracing::info!("Add Helm repos"); // Stable repository helm::add_repo(HELM_REPO_NAME_STABLE, &self.repos.helm_repo_stable)?; @@ -151,7 +150,6 @@ impl Cli { Ok(()) } - #[instrument] pub fn cache_settings(&self) -> Result { if self.no_cache { Ok(Settings::disabled()) @@ -167,7 +165,7 @@ impl Cli { } } - #[instrument] + #[instrument(skip_all)] pub async fn run(&self) -> Result { // FIXME (Techassi): There might be a better way to handle this with // the match later in this function. diff --git a/rust/stackablectl/src/cmds/demo.rs b/rust/stackablectl/src/cmds/demo.rs index 437977a6..43d7e0af 100644 --- a/rust/stackablectl/src/cmds/demo.rs +++ b/rust/stackablectl/src/cmds/demo.rs @@ -160,7 +160,7 @@ pub enum CmdError { } impl DemoArgs { - #[instrument] + #[instrument(skip_all, fields(with_cache = !cli.no_cache))] pub async fn run(&self, cli: &Cli, cache: Cache) -> Result { debug!("Handle demo args"); @@ -211,7 +211,7 @@ impl DemoArgs { } /// Print out a list of demos, either as a table (plain), JSON or YAML -#[instrument] +#[instrument(skip_all)] async fn list_cmd(args: &DemoListArgs, cli: &Cli, list: demo::List) -> Result { info!("Listing demos"); @@ -259,7 +259,7 @@ async fn list_cmd(args: &DemoListArgs, cli: &Cli, list: demo::List) -> Result Result { debug!("Listing operators"); @@ -244,7 +244,7 @@ async fn list_cmd(args: &OperatorListArgs, cli: &Cli) -> Result Result { debug!("Describing operator {}", args.operator_name); @@ -303,7 +303,7 @@ async fn describe_cmd(args: &OperatorDescribeArgs, cli: &Cli) -> Result Result { info!("Installing operator(s)"); @@ -351,7 +351,7 @@ async fn install_cmd(args: &OperatorInstallArgs, cli: &Cli) -> Result Result { info!("Uninstalling operator(s)"); @@ -381,7 +381,7 @@ fn uninstall_cmd(args: &OperatorUninstallArgs, cli: &Cli) -> Result Result { debug!("Listing installed operators"); @@ -493,7 +493,7 @@ async fn build_source_index_file_list<'a>( /// Iterates over all valid operators and creates a list of versions grouped /// by stable, test and dev lines based on the list of Helm repo index files. -#[instrument] +#[instrument(skip_all)] fn build_versions_list( helm_index_files: &HashMap<&str, ChartSourceMetadata>, ) -> Result, CmdError> { @@ -515,13 +515,13 @@ fn build_versions_list( /// Builds a list of versions for one operator (by name) which is grouped by /// stable, test and dev lines based on the list of Helm repo index files. -#[instrument] +#[instrument(skip_all, fields(%operator_name))] fn build_versions_list_for_operator( operator_name: T, helm_index_files: &HashMap<&str, ChartSourceMetadata>, ) -> Result where - T: AsRef + std::fmt::Debug, + T: AsRef + std::fmt::Display + std::fmt::Debug, { debug!( "Build versions list for operator {}", diff --git a/rust/stackablectl/src/cmds/stack.rs b/rust/stackablectl/src/cmds/stack.rs index 7391f1f9..3de90344 100644 --- a/rust/stackablectl/src/cmds/stack.rs +++ b/rust/stackablectl/src/cmds/stack.rs @@ -193,7 +193,7 @@ impl StackArgs { } } -#[instrument] +#[instrument(skip_all)] fn list_cmd( args: &StackListArgs, cli: &Cli, @@ -243,7 +243,7 @@ fn list_cmd( } } -#[instrument] +#[instrument(skip_all)] fn describe_cmd( args: &StackDescribeArgs, cli: &Cli, diff --git a/rust/stackablectl/src/cmds/stacklet.rs b/rust/stackablectl/src/cmds/stacklet.rs index 55c3b6e8..420d704f 100644 --- a/rust/stackablectl/src/cmds/stacklet.rs +++ b/rust/stackablectl/src/cmds/stacklet.rs @@ -90,7 +90,7 @@ impl StackletArgs { } } -#[instrument] +#[instrument(skip_all)] async fn list_cmd(args: &StackletListArgs, cli: &Cli) -> Result { info!("Listing installed stacklets"); @@ -203,7 +203,7 @@ async fn list_cmd(args: &StackletListArgs, cli: &Cli) -> Result Result { info!("Displaying stacklet credentials"); From 74f9a4b0ec797f8750ea6b8b714d0bfe2c8fa436 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 13:45:37 +0100 Subject: [PATCH 02/11] chore(instrumentation): Improve trace events with fields --- .../src/platform/operator/mod.rs | 2 +- .../src/platform/release/spec.rs | 40 +++++++++++-------- .../src/platform/stack/spec.rs | 2 +- rust/stackablectl/src/cli/mod.rs | 8 +++- rust/stackablectl/src/cmds/demo.rs | 4 +- rust/stackablectl/src/cmds/operator.rs | 11 +++-- rust/stackablectl/src/cmds/release.rs | 3 +- rust/stackablectl/src/cmds/stack.rs | 4 +- 8 files changed, 44 insertions(+), 30 deletions(-) diff --git a/rust/stackable-cockpit/src/platform/operator/mod.rs b/rust/stackable-cockpit/src/platform/operator/mod.rs index 556943bc..645936dc 100644 --- a/rust/stackable-cockpit/src/platform/operator/mod.rs +++ b/rust/stackable-cockpit/src/platform/operator/mod.rs @@ -191,7 +191,7 @@ impl OperatorSpec { namespace: &str, chart_source: &ChartSourceType, ) -> Result<(), helm::Error> { - info!("Installing operator {}", self); + info!(operator = %self, "Installing operator"); let version = self.version.as_ref().map(|v| v.to_string()); let helm_name = self.helm_name(); diff --git a/rust/stackable-cockpit/src/platform/release/spec.rs b/rust/stackable-cockpit/src/platform/release/spec.rs index bcdf5ccc..4fa0a42d 100644 --- a/rust/stackable-cockpit/src/platform/release/spec.rs +++ b/rust/stackable-cockpit/src/platform/release/spec.rs @@ -70,26 +70,34 @@ impl ReleaseSpec { let namespace = namespace.to_string(); futures::stream::iter(self.filter_products(include_products, exclude_products)) .map(|(product_name, product)| { + let task_span = + tracing::debug_span!("install_operator", product_name = tracing::field::Empty); + let namespace = namespace.clone(); let chart_source = chart_source.clone(); // Helm installs currently `block_in_place`, so we need to spawn each job onto a separate task to // get useful parallelism. - tokio::spawn(async move { - info!("Installing {product_name}-operator"); - - // Create operator spec - let operator = OperatorSpec::new(&product_name, Some(product.version.clone())) - .context(OperatorSpecParseSnafu)?; - - // Install operator - operator - .install(&namespace, &chart_source) - .context(HelmInstallSnafu)?; - - info!("Installed {product_name}-operator"); - - Ok(()) - }) + tokio::spawn( + async move { + Span::current().record("product_name", &product_name); + info!("Installing {product_name}-operator"); + + // Create operator spec + let operator = + OperatorSpec::new(&product_name, Some(product.version.clone())) + .context(OperatorSpecParseSnafu)?; + + // Install operator + operator + .install(&namespace, &chart_source) + .context(HelmInstallSnafu)?; + + info!("Installed {product_name}-operator"); + + Ok(()) + } + .instrument(task_span), + ) }) .buffer_unordered(10) .map(|res| res.context(BackgroundTaskSnafu)?) diff --git a/rust/stackable-cockpit/src/platform/stack/spec.rs b/rust/stackable-cockpit/src/platform/stack/spec.rs index 2fd712e1..87262153 100644 --- a/rust/stackable-cockpit/src/platform/stack/spec.rs +++ b/rust/stackable-cockpit/src/platform/stack/spec.rs @@ -206,7 +206,7 @@ impl StackSpec { _product_namespace: &str, // TODO (@NickLarsenNZ): remove this field chart_source: &ChartSourceType, ) -> Result<(), Error> { - info!("Trying to install release {}", self.release); + info!(self.release, "Trying to install release"); // Get the release by name let release = release_list diff --git a/rust/stackablectl/src/cli/mod.rs b/rust/stackablectl/src/cli/mod.rs index 15e48053..60c035d1 100644 --- a/rust/stackablectl/src/cli/mod.rs +++ b/rust/stackablectl/src/cli/mod.rs @@ -152,6 +152,7 @@ impl Cli { pub fn cache_settings(&self) -> Result { if self.no_cache { + tracing::debug!("Cache disabled"); Ok(Settings::disabled()) } else { let project_dir = ProjectDirs::from( @@ -161,7 +162,12 @@ impl Cli { ) .ok_or(CacheSettingsError::UserDir)?; - Ok(Settings::disk(project_dir.cache_dir())) + let cache_dir = project_dir.cache_dir(); + tracing::debug!( + cache_dir = %cache_dir.to_string_lossy(), + "Setting cache directory" + ); + Ok(Settings::disk(cache_dir)) } } diff --git a/rust/stackablectl/src/cmds/demo.rs b/rust/stackablectl/src/cmds/demo.rs index 43d7e0af..b98ed723 100644 --- a/rust/stackablectl/src/cmds/demo.rs +++ b/rust/stackablectl/src/cmds/demo.rs @@ -265,7 +265,7 @@ async fn describe_cmd( cli: &Cli, list: demo::List, ) -> Result { - info!("Describing demo {}", args.demo_name); + info!(demo_name = %args.demo_name, "Describing demo"); let demo = list.get(&args.demo_name).ok_or(CmdError::NoSuchDemo { name: args.demo_name.clone(), @@ -327,7 +327,7 @@ async fn install_cmd( transfer_client: &xfer::Client, release_branch: &str, ) -> Result { - info!(%release_branch, "Installing demo {}", args.demo_name); + info!(demo_name = %args.demo_name, "Installing demo"); // Init result output and progress output let mut output = cli.result(); diff --git a/rust/stackablectl/src/cmds/operator.rs b/rust/stackablectl/src/cmds/operator.rs index b38311d2..0054fefc 100644 --- a/rust/stackablectl/src/cmds/operator.rs +++ b/rust/stackablectl/src/cmds/operator.rs @@ -246,7 +246,7 @@ async fn list_cmd(args: &OperatorListArgs, cli: &Cli) -> Result Result { - debug!("Describing operator {}", args.operator_name); + debug!(operator_name = %args.operator_name, "Describing operator"); // Build map which maps artifacts to a chart source let source_index_files = @@ -328,6 +328,8 @@ async fn install_cmd(args: &OperatorInstallArgs, cli: &Cli) -> Result Result Result { - debug!("Listing installed operators"); + info!("Listing installed operators"); type ReleaseList = IndexMap; @@ -523,10 +525,7 @@ fn build_versions_list_for_operator( where T: AsRef + std::fmt::Display + std::fmt::Debug, { - debug!( - "Build versions list for operator {}", - operator_name.as_ref() - ); + debug!("Build versions list for operator"); let mut versions_list = OperatorVersionList(HashMap::new()); let operator_name = operator_name.as_ref(); diff --git a/rust/stackablectl/src/cmds/release.rs b/rust/stackablectl/src/cmds/release.rs index e0dca6fd..698dcdd8 100644 --- a/rust/stackablectl/src/cmds/release.rs +++ b/rust/stackablectl/src/cmds/release.rs @@ -211,7 +211,7 @@ async fn describe_cmd( cli: &Cli, release_list: release::ReleaseList, ) -> Result { - info!("Describing release"); + info!(release = %args.release, "Describing release"); let release = release_list.get(&args.release); @@ -272,6 +272,7 @@ async fn install_cmd( cli: &Cli, release_list: release::ReleaseList, ) -> Result { + info!(release = %args.release, "Installing release"); match release_list.get(&args.release) { Some(release) => { let mut output = cli.result(); diff --git a/rust/stackablectl/src/cmds/stack.rs b/rust/stackablectl/src/cmds/stack.rs index 3de90344..7f357c49 100644 --- a/rust/stackablectl/src/cmds/stack.rs +++ b/rust/stackablectl/src/cmds/stack.rs @@ -249,7 +249,7 @@ fn describe_cmd( cli: &Cli, stack_list: stack::StackList, ) -> Result { - info!("Describing stack {}", args.stack_name); + info!(stack_name = %args.stack_name, "Describing stack"); match stack_list.get(&args.stack_name) { Some(stack) => match args.output_type { @@ -312,7 +312,7 @@ async fn install_cmd( stack_list: stack::StackList, transfer_client: &xfer::Client, ) -> Result { - info!("Installing stack {}", args.stack_name); + info!(stack_name = %args.stack_name, "Installing stack"); let files = cli.get_release_files().context(PathOrUrlParseSnafu)?; let release_list = release::ReleaseList::build(&files, transfer_client) From e3f3ed285b46c540e4565db6695016d001ca7044 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 14:59:39 +0100 Subject: [PATCH 03/11] chore(instrumentation): Further improvements of tracing events --- rust/stackable-cockpit/src/oci.rs | 23 +++++++++-------------- rust/stackablectl/src/cmds/operator.rs | 4 ++-- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/rust/stackable-cockpit/src/oci.rs b/rust/stackable-cockpit/src/oci.rs index 0c8eea63..dda17b13 100644 --- a/rust/stackable-cockpit/src/oci.rs +++ b/rust/stackable-cockpit/src/oci.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use serde::Deserialize; use snafu::{OptionExt, ResultExt, Snafu}; -use tracing::debug; +use tracing::{debug, instrument}; use url::Url; use urlencoding::encode; @@ -117,6 +117,8 @@ impl OciUrlExt for Url { } } +// TODO (@NickLarsenNZ): Look into why a HashMap is used here when the key is inside each entry in the value +#[instrument] pub async fn get_oci_index<'a>() -> Result, Error> { let mut source_index_files: HashMap<&str, ChartSourceMetadata> = HashMap::new(); @@ -153,7 +155,10 @@ pub async fn get_oci_index<'a>() -> Result .await .context(ParseRepositoriesSnafu)?; - debug!("OCI repos {:?}", repositories); + debug!( + count = repositories.len(), + "Received response for OCI repositories" + ); for repository in &repositories { // fetch all artifacts pro operator @@ -162,7 +167,7 @@ pub async fn get_oci_index<'a>() -> Result .split_once('/') .context(UnexpectedOciRepositoryNameSnafu)?; - debug!("OCI repo parts {} and {}", project_name, repository_name); + tracing::trace!(project_name, repository_name, "OCI repository parts"); let mut artifacts = Vec::new(); let mut page = 1; @@ -196,17 +201,7 @@ pub async fn get_oci_index<'a>() -> Result .replace("-arm64", "") .replace("-amd64", ""); - debug!( - "OCI resolved artifact {}, {}, {}", - release_version.to_string(), - repository_name.to_string(), - release_artifact.name.to_string() - ); - - debug!( - "Repo/Artifact/Tag: {:?} / {:?} / {:?}", - repository, artifact, release_artifact - ); + tracing::trace!(repository_name, release_version, "OCI resolved artifact"); let entry = ChartSourceEntry { name: repository_name.to_string(), diff --git a/rust/stackablectl/src/cmds/operator.rs b/rust/stackablectl/src/cmds/operator.rs index 0054fefc..5e44403b 100644 --- a/rust/stackablectl/src/cmds/operator.rs +++ b/rust/stackablectl/src/cmds/operator.rs @@ -539,13 +539,13 @@ where } /// Builds a list of operator versions based on the provided Helm repo. -#[instrument] +#[instrument(skip_all, fields(%operator_name))] fn list_operator_versions_from_repo( operator_name: T, helm_repo: &ChartSourceMetadata, ) -> Result, CmdError> where - T: AsRef + std::fmt::Debug, + T: AsRef + std::fmt::Display + std::fmt::Debug, { debug!("Listing operator versions from repo"); From fd232982a0a66cb0e99d8eef5655ae06fdd0b3f3 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 15:00:12 +0100 Subject: [PATCH 04/11] chore: Clean up format! --- rust/stackable-cockpit/src/oci.rs | 6 +++--- rust/stackablectl/src/cmds/operator.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/rust/stackable-cockpit/src/oci.rs b/rust/stackable-cockpit/src/oci.rs index dda17b13..980d002c 100644 --- a/rust/stackable-cockpit/src/oci.rs +++ b/rust/stackable-cockpit/src/oci.rs @@ -135,12 +135,12 @@ pub async fn get_oci_index<'a>() -> Result }, ); } - let base_url = format!("https://{}/api/v2.0", HELM_OCI_BASE); + let base_url = format!("https://{HELM_OCI_BASE}/api/v2.0"); // fetch all operators let url = format!( - "{}/repositories?page_size={}&q=name=~sdp-charts/", - base_url, 100 + "{base_url}/repositories?page_size={page_size}&q=name=~sdp-charts/", + page_size = 100 ); // reuse connections diff --git a/rust/stackablectl/src/cmds/operator.rs b/rust/stackablectl/src/cmds/operator.rs index 5e44403b..4cb35f0d 100644 --- a/rust/stackablectl/src/cmds/operator.rs +++ b/rust/stackablectl/src/cmds/operator.rs @@ -467,7 +467,7 @@ async fn build_source_index_file_list<'a>( ChartSourceType::OCI => { source_index_files = oci::get_oci_index().await.context(OciSnafu)?; - debug!("OCI Repository entries: {:?}", source_index_files); + debug!(count = source_index_files.len(), "OCI Repository entries"); } ChartSourceType::Repo => { for helm_repo_name in [ From 100669a3773b70d841461c857e52de59f38d6d8f Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 15:04:31 +0100 Subject: [PATCH 05/11] chore(instrumentation): Remove unused import --- rust/stackablectl/src/cli/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/stackablectl/src/cli/mod.rs b/rust/stackablectl/src/cli/mod.rs index 60c035d1..7bef7840 100644 --- a/rust/stackablectl/src/cli/mod.rs +++ b/rust/stackablectl/src/cli/mod.rs @@ -3,7 +3,7 @@ use std::env; use clap::{Parser, Subcommand, ValueEnum}; use directories::ProjectDirs; use snafu::{ResultExt, Snafu}; -use tracing::{debug, instrument, Level}; +use tracing::{instrument, Level}; use stackable_cockpit::{ constants::{HELM_REPO_NAME_DEV, HELM_REPO_NAME_STABLE, HELM_REPO_NAME_TEST}, From cf2c87c6a6629cba7cee55a2e298fc302cd84f4e Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 20 Mar 2025 15:07:48 +0100 Subject: [PATCH 06/11] chore(instrumentation): Add comments for future improvements --- rust/helm-sys/src/lib.rs | 2 ++ rust/stackable-cockpit/src/utils/chartsource.rs | 3 +++ rust/stackablectl/src/cmds/operator.rs | 13 +++++++++++++ 3 files changed, 18 insertions(+) diff --git a/rust/helm-sys/src/lib.rs b/rust/helm-sys/src/lib.rs index 1e934586..9c7d66d4 100644 --- a/rust/helm-sys/src/lib.rs +++ b/rust/helm-sys/src/lib.rs @@ -56,6 +56,8 @@ pub fn uninstall_helm_release( } } +// TODO (@NickLarsenNZ): Add tracing to helm-sys, maybe? +// #[instrument] pub fn check_helm_release_exists(release_name: &str, namespace: &str) -> bool { let release_name = CString::new(release_name).unwrap(); let namespace = CString::new(namespace).unwrap(); diff --git a/rust/stackable-cockpit/src/utils/chartsource.rs b/rust/stackable-cockpit/src/utils/chartsource.rs index 5a829e04..d84c1a13 100644 --- a/rust/stackable-cockpit/src/utils/chartsource.rs +++ b/rust/stackable-cockpit/src/utils/chartsource.rs @@ -4,6 +4,9 @@ use serde::Deserialize; #[derive(Clone, Debug, Deserialize)] pub struct ChartSourceMetadata { + // TODO (@NickLarsenNZ): Add a field for the repo name. + // This would remove the need for the nested HashMaps + // See: around rust/stackablectl/src/cmds/operator.rs:519 pub entries: HashMap>, } diff --git a/rust/stackablectl/src/cmds/operator.rs b/rust/stackablectl/src/cmds/operator.rs index 4cb35f0d..e522e769 100644 --- a/rust/stackablectl/src/cmds/operator.rs +++ b/rust/stackablectl/src/cmds/operator.rs @@ -468,6 +468,19 @@ async fn build_source_index_file_list<'a>( source_index_files = oci::get_oci_index().await.context(OciSnafu)?; debug!(count = source_index_files.len(), "OCI Repository entries"); + + // TODO (@NickLarsenNZ): Look into why this is so deeply nested with duplicate data. + // source_index_files + // .iter() + // .for_each(|(&repo_name, chart_source_metadata)| { + // let x = chart_source_metadata.entries.len(); + // tracing::trace!(repo_name, x, "thing"); + // let _ = &chart_source_metadata + // .entries + // .iter() + // // y (below) is a Vec + // .for_each(|(x, y)| tracing::error!(x, "blah {:?}", y)); + // }); } ChartSourceType::Repo => { for helm_repo_name in [ From 2c72e645a38510e5034bf78d333facb448322291 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 21 Mar 2025 13:49:47 +0100 Subject: [PATCH 07/11] chore(instrumentation): Tidy up helm repo list --- rust/stackablectl/src/cmds/operator.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/rust/stackablectl/src/cmds/operator.rs b/rust/stackablectl/src/cmds/operator.rs index e522e769..4616b966 100644 --- a/rust/stackablectl/src/cmds/operator.rs +++ b/rust/stackablectl/src/cmds/operator.rs @@ -518,7 +518,13 @@ fn build_versions_list( for operator in operator::VALID_OPERATORS { for (helm_repo_name, helm_repo_index_file) in helm_index_files { - let versions = list_operator_versions_from_repo(operator, helm_repo_index_file)?; + let span = tracing::info_span!( + "build_versions_list_iter", + helm.repository.name = %helm_repo_name, + operator_name = %operator, + ); + let versions = + span.in_scope(|| list_operator_versions_from_repo(operator, helm_repo_index_file))?; let entry = versions_list.entry(operator.to_string()); let entry = entry.or_insert(OperatorVersionList(HashMap::new())); entry.0.insert(helm_repo_name.to_string(), versions); @@ -560,15 +566,16 @@ fn list_operator_versions_from_repo( where T: AsRef + std::fmt::Display + std::fmt::Debug, { - debug!("Listing operator versions from repo"); + debug!("Listing operator versions from repository"); - let operator_name = utils::operator_chart_name(operator_name.as_ref()); + let chart_name = utils::operator_chart_name(operator_name.as_ref()); - match helm_repo.entries.get(&operator_name) { + match helm_repo.entries.get(&chart_name) { Some(entries) => { let mut versions = entries .iter() .map(|entry| { + tracing::trace!(helm.chart.name = %chart_name, helm.chart.version = %entry.version, "Found operator chart version"); Version::parse(&entry.version).with_context(|_| InvalidHelmChartVersionSnafu { version: entry.version.clone(), }) From 0eb710acde925a9c128271fd6375a57557b3311a Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 21 Mar 2025 14:07:42 +0100 Subject: [PATCH 08/11] chore(instrumentation): Add comment for future change --- rust/stackable-cockpit/src/oci.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust/stackable-cockpit/src/oci.rs b/rust/stackable-cockpit/src/oci.rs index 980d002c..337cc876 100644 --- a/rust/stackable-cockpit/src/oci.rs +++ b/rust/stackable-cockpit/src/oci.rs @@ -162,6 +162,7 @@ pub async fn get_oci_index<'a>() -> Result for repository in &repositories { // fetch all artifacts pro operator + // NOTE (@NickLarsenNZ): I think repository_name should be helm_chart_name. let (project_name, repository_name) = repository .name .split_once('/') From 6b9a0ada866bb644b573d0743845a47b16e7b9e3 Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 21 Mar 2025 14:10:04 +0100 Subject: [PATCH 09/11] chore(stackablectl): Update changelog --- rust/stackablectl/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/stackablectl/CHANGELOG.md b/rust/stackablectl/CHANGELOG.md index 52247895..c5743084 100644 --- a/rust/stackablectl/CHANGELOG.md +++ b/rust/stackablectl/CHANGELOG.md @@ -8,9 +8,11 @@ All notable changes to this project will be documented in this file. - Use `rustls-native-certs` so that `stackablectl` can be used in environments with internal PKI ([#351]). - Use `heritage` label when looking up the `minio-console` stacklet ([#364]). +- Improve tracing and log output ([#365]). [#351]: https://github.com/stackabletech/stackable-cockpit/pull/351 [#364]: https://github.com/stackabletech/stackable-cockpit/pull/364 +[#365]: https://github.com/stackabletech/stackable-cockpit/pull/365 ## [24.11.3] - 2025-01-27 From 602850ac2fddb6bb6f292e75be3d55247dc83dec Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Fri, 21 Mar 2025 14:17:12 +0100 Subject: [PATCH 10/11] chore(instrumentation): Fix clippy lints --- rust/stackable-cockpit/src/platform/operator/mod.rs | 2 +- rust/stackable-cockpit/src/platform/stack/spec.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/stackable-cockpit/src/platform/operator/mod.rs b/rust/stackable-cockpit/src/platform/operator/mod.rs index 645936dc..5adbb768 100644 --- a/rust/stackable-cockpit/src/platform/operator/mod.rs +++ b/rust/stackable-cockpit/src/platform/operator/mod.rs @@ -184,7 +184,7 @@ impl OperatorSpec { // NOTE (@NickLarsenNZ): Option doesn't impl Display, so we need to call // display for the inner type if it exists. Otherwise we gte the Debug // impl for the whole Option. - version = self.version.as_ref().map(|v| tracing::field::display(v)), + version = self.version.as_ref().map(tracing::field::display), ))] pub fn install( &self, diff --git a/rust/stackable-cockpit/src/platform/stack/spec.rs b/rust/stackable-cockpit/src/platform/stack/spec.rs index 87262153..3cd839fc 100644 --- a/rust/stackable-cockpit/src/platform/stack/spec.rs +++ b/rust/stackable-cockpit/src/platform/stack/spec.rs @@ -156,7 +156,7 @@ impl StackSpec { // NOTE (@NickLarsenNZ): Option doesn't impl Display, so we need to call // display for the inner type if it exists. Otherwise we gte the Debug // impl for the whole Option. - demo_name = install_parameters.demo_name.as_ref().map(|v| tracing::field::display(v)), + demo_name = install_parameters.demo_name.as_ref().map(tracing::field::display), ))] pub async fn install( &self, From 88684a2979bc489b0230dc5906bf9e08f8d5cd3d Mon Sep 17 00:00:00 2001 From: Nick Larsen Date: Thu, 27 Mar 2025 09:54:32 +0100 Subject: [PATCH 11/11] chore(instrumentation): Split instrumentation fields across mutliple lines --- rust/stackable-cockpit/src/platform/release/spec.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rust/stackable-cockpit/src/platform/release/spec.rs b/rust/stackable-cockpit/src/platform/release/spec.rs index 4fa0a42d..eb815e55 100644 --- a/rust/stackable-cockpit/src/platform/release/spec.rs +++ b/rust/stackable-cockpit/src/platform/release/spec.rs @@ -50,7 +50,11 @@ pub struct ReleaseSpec { impl ReleaseSpec { /// Installs a release by installing individual operators. - #[instrument(skip_all, fields(%namespace, product.included = tracing::field::Empty, product.excluded = tracing::field::Empty))] + #[instrument(skip_all, fields( + %namespace, + product.included = tracing::field::Empty, + product.excluded = tracing::field::Empty, + ))] pub async fn install( &self, include_products: &[String],