chore(instrumentation): Reduce noise from instrumented functions (#365)

* chore(instrumentation): Reduce noise from instrumented functions

* chore(instrumentation): Improve trace events with fields

* chore(instrumentation): Further improvements of tracing events

* chore: Clean up format!

* chore(instrumentation): Remove unused import

* chore(instrumentation): Add comments for future improvements

* chore(instrumentation): Tidy up helm repo list

* chore(instrumentation): Add comment for future change

* chore(stackablectl): Update changelog

* chore(instrumentation): Fix clippy lints

* chore(instrumentation): Split instrumentation fields across mutliple lines

Author: NickLarsenNZ

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();

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");
 

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<bool> {
         debug!("Checking if kind cluster exists");
 

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<bool, Error> {
         debug!("Checking if Minikube cluster exists");
 

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: `<PRODUCT_NAME>-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: `<PRODUCT_NAME>-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<T>(repo_url: T) -> Result<ChartSourceMetadata, Error>
 where
-    T: AsRef<str> + std::fmt::Debug,
+    T: AsRef<str> + std::fmt::Display + std::fmt::Debug,
 {
     debug!("Get Helm repo index file");
 

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<HashMap<&'a str, ChartSourceMetadata>, Error> {
     let mut source_index_files: HashMap<&str, ChartSourceMetadata> = HashMap::new();
 
@@ -133,12 +135,12 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
             },
         );
     }
-    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
@@ -153,16 +155,20 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
         .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
+        // NOTE (@NickLarsenNZ): I think repository_name should be helm_chart_name.
         let (project_name, repository_name) = repository
             .name
             .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 +202,7 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
                     .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(),

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,

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 =

rust/stackable-cockpit/src/platform/operator/mod.rs

@@ -178,13 +178,20 @@ 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(tracing::field::display),
+    ))]
     pub fn install(
         &self,
         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();
@@ -213,10 +220,10 @@ impl OperatorSpec {
     }
 
     /// Uninstalls the operator using Helm.
-    #[instrument]
+    #[instrument(skip_all, fields(%namespace))]
     pub fn uninstall<T>(&self, namespace: T) -> Result<(), helm::Error>
     where
-        T: AsRef<str> + std::fmt::Debug,
+        T: AsRef<str> + std::fmt::Display + std::fmt::Debug,
     {
         match helm::uninstall_release(&self.helm_name(), namespace.as_ref(), true) {
             Ok(status) => {

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,11 @@ 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,29 +64,44 @@ 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)| {
+                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)?)