Skip to content

Commit 063f4c3

Browse files
NickLarsenNZNickLarsenNZ
authored
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
1 parent 3604c1e commit 063f4c3

File tree

20 files changed

+176
-106
lines changed

20 files changed

+176
-106
lines changed

‎rust/helm-sys/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ pub fn uninstall_helm_release(
5656
}
5757
}
5858

59+
// TODO (@NickLarsenNZ): Add tracing to helm-sys, maybe?
60+
// #[instrument]
5961
pub fn check_helm_release_exists(release_name: &str, namespace: &str) -> bool {
6062
let release_name = CString::new(release_name).unwrap();
6163
let namespace = CString::new(namespace).unwrap();

‎rust/stackable-cockpit/src/engine/docker/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ pub enum Error {
1919
}
2020

2121
/// Checks if Docker is running on the system
22-
#[instrument]
22+
#[instrument(skip_all)]
2323
pub async fn check_if_docker_is_running() -> Result<()> {
2424
debug!("Checking if Docker is running");
2525

‎rust/stackable-cockpit/src/engine/kind/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ impl Cluster {
6464
}
6565

6666
/// Create a new local cluster by calling the kind binary.
67-
#[instrument]
67+
#[instrument(skip_all)]
6868
pub async fn create(&self) -> Result<()> {
6969
info!("Creating local cluster using kind");
7070

@@ -109,7 +109,7 @@ impl Cluster {
109109
}
110110

111111
/// Creates a kind cluster if it doesn't exist already.
112-
#[instrument]
112+
#[instrument(skip_all)]
113113
pub async fn create_if_not_exists(&self) -> Result<()> {
114114
info!("Creating cluster if it doesn't exist using kind");
115115

@@ -131,7 +131,7 @@ impl Cluster {
131131
}
132132

133133
/// Check if a kind cluster with the provided name already exists.
134-
#[instrument]
134+
#[instrument(skip_all)]
135135
async fn check_if_cluster_exists(cluster_name: &str) -> Result<bool> {
136136
debug!("Checking if kind cluster exists");
137137

‎rust/stackable-cockpit/src/engine/minikube/mod.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ impl Cluster {
4141
}
4242

4343
/// Create a new local cluster by calling the Minikube binary
44-
#[instrument]
44+
#[instrument(skip_all)]
4545
pub async fn create(&self) -> Result<(), Error> {
4646
info!("Creating local cluster using Minikube");
4747

@@ -70,7 +70,7 @@ impl Cluster {
7070
}
7171

7272
/// Creates a Minikube cluster if it doesn't exist already.
73-
#[instrument]
73+
#[instrument(skip_all)]
7474
pub async fn create_if_not_exists(&self) -> Result<(), Error> {
7575
info!("Creating cluster if it doesn't exist using Minikube");
7676

@@ -92,7 +92,7 @@ impl Cluster {
9292
}
9393

9494
/// Check if a kind cluster with the provided name already exists.
95-
#[instrument]
95+
#[instrument(skip_all)]
9696
async fn check_if_cluster_exists(cluster_name: &str) -> Result<bool, Error> {
9797
debug!("Checking if Minikube cluster exists");
9898

‎rust/stackable-cockpit/src/helm.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ pub struct ChartVersion<'a> {
182182
///
183183
/// This function expects the fully qualified Helm release name. In case of our
184184
/// operators this is: `<PRODUCT_NAME>-operator`.
185-
#[instrument]
185+
#[instrument(skip(values_yaml), fields(with_values = values_yaml.is_some()))]
186186
pub fn install_release_from_repo_or_registry(
187187
release_name: &str,
188188
ChartVersion {
@@ -239,8 +239,8 @@ pub fn install_release_from_repo_or_registry(
239239
let chart_version = chart_version.unwrap_or(HELM_DEFAULT_CHART_VERSION);
240240

241241
debug!(
242-
"Installing Helm release {} ({}) from chart {}",
243-
release_name, chart_version, full_chart_name
242+
release_name,
243+
chart_version, full_chart_name, "Installing Helm release"
244244
);
245245

246246
install_release(
@@ -260,6 +260,7 @@ pub fn install_release_from_repo_or_registry(
260260
///
261261
/// This function expects the fully qualified Helm release name. In case of our
262262
/// operators this is: `<PRODUCT_NAME>-operator`.
263+
#[instrument(fields(with_values = values_yaml.is_some()))]
263264
fn install_release(
264265
release_name: &str,
265266
chart_name: &str,
@@ -388,10 +389,10 @@ pub fn add_repo(repository_name: &str, repository_url: &str) -> Result<(), Error
388389
}
389390

390391
/// Retrieves the Helm index file from the repository URL.
391-
#[instrument]
392+
#[instrument(skip_all, fields(%repo_url))]
392393
pub async fn get_helm_index<T>(repo_url: T) -> Result<ChartSourceMetadata, Error>
393394
where
394-
T: AsRef<str> + std::fmt::Debug,
395+
T: AsRef<str> + std::fmt::Display + std::fmt::Debug,
395396
{
396397
debug!("Get Helm repo index file");
397398

‎rust/stackable-cockpit/src/oci.rs

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use std::collections::HashMap;
22

33
use serde::Deserialize;
44
use snafu::{OptionExt, ResultExt, Snafu};
5-
use tracing::debug;
5+
use tracing::{debug, instrument};
66
use url::Url;
77
use urlencoding::encode;
88

@@ -117,6 +117,8 @@ impl OciUrlExt for Url {
117117
}
118118
}
119119

120+
// TODO (@NickLarsenNZ): Look into why a HashMap is used here when the key is inside each entry in the value
121+
#[instrument]
120122
pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>, Error> {
121123
let mut source_index_files: HashMap<&str, ChartSourceMetadata> = HashMap::new();
122124

@@ -133,12 +135,12 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
133135
},
134136
);
135137
}
136-
let base_url = format!("https://{}/api/v2.0", HELM_OCI_BASE);
138+
let base_url = format!("https://{HELM_OCI_BASE}/api/v2.0");
137139

138140
// fetch all operators
139141
let url = format!(
140-
"{}/repositories?page_size={}&q=name=~sdp-charts/",
141-
base_url, 100
142+
"{base_url}/repositories?page_size={page_size}&q=name=~sdp-charts/",
143+
page_size = 100
142144
);
143145

144146
// reuse connections
@@ -153,16 +155,20 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
153155
.await
154156
.context(ParseRepositoriesSnafu)?;
155157

156-
debug!("OCI repos {:?}", repositories);
158+
debug!(
159+
count = repositories.len(),
160+
"Received response for OCI repositories"
161+
);
157162

158163
for repository in &repositories {
159164
// fetch all artifacts pro operator
165+
// NOTE (@NickLarsenNZ): I think repository_name should be helm_chart_name.
160166
let (project_name, repository_name) = repository
161167
.name
162168
.split_once('/')
163169
.context(UnexpectedOciRepositoryNameSnafu)?;
164170

165-
debug!("OCI repo parts {} and {}", project_name, repository_name);
171+
tracing::trace!(project_name, repository_name, "OCI repository parts");
166172

167173
let mut artifacts = Vec::new();
168174
let mut page = 1;
@@ -196,17 +202,7 @@ pub async fn get_oci_index<'a>() -> Result<HashMap<&'a str, ChartSourceMetadata>
196202
.replace("-arm64", "")
197203
.replace("-amd64", "");
198204

199-
debug!(
200-
"OCI resolved artifact {}, {}, {}",
201-
release_version.to_string(),
202-
repository_name.to_string(),
203-
release_artifact.name.to_string()
204-
);
205-
206-
debug!(
207-
"Repo/Artifact/Tag: {:?} / {:?} / {:?}",
208-
repository, artifact, release_artifact
209-
);
205+
tracing::trace!(repository_name, release_version, "OCI resolved artifact");
210206

211207
let entry = ChartSourceEntry {
212208
name: repository_name.to_string(),

‎rust/stackable-cockpit/src/platform/demo/spec.rs

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,11 @@ impl DemoSpec {
134134
Ok(())
135135
}
136136

137+
#[instrument(skip_all, fields(
138+
stack_name = %self.stack,
139+
operator_namespace = %install_parameters.operator_namespace,
140+
product_namespace = %install_parameters.product_namespace,
141+
))]
137142
pub async fn install(
138143
&self,
139144
stack_list: StackList,
@@ -177,7 +182,11 @@ impl DemoSpec {
177182
.await
178183
}
179184

180-
#[instrument(skip_all)]
185+
#[instrument(skip_all, fields(
186+
stack_name = %self.stack,
187+
operator_namespace = %install_params.operator_namespace,
188+
product_namespace = %install_params.product_namespace,
189+
))]
181190
async fn prepare_manifests(
182191
&self,
183192
install_params: DemoInstallParameters,

‎rust/stackable-cockpit/src/platform/manifests.rs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ pub enum Error {
6262

6363
pub trait InstallManifestsExt {
6464
// TODO (Techassi): This step shouldn't care about templating the manifests nor fetching them from remote
65-
#[instrument(skip_all)]
65+
#[instrument(skip_all, fields(%product_namespace))]
6666
#[allow(async_fn_in_trait)]
6767
async fn install_manifests(
6868
manifests: &[ManifestSpec],
@@ -72,12 +72,12 @@ pub trait InstallManifestsExt {
7272
client: &Client,
7373
transfer_client: &xfer::Client,
7474
) -> Result<(), Error> {
75-
debug!("Installing demo / stack manifests");
75+
debug!("Installing manifests");
7676

7777
for manifest in manifests {
7878
match manifest {
7979
ManifestSpec::HelmChart(helm_file) => {
80-
debug!("Installing manifest from Helm chart {}", helm_file);
80+
debug!(helm_file, "Installing manifest from Helm chart");
8181

8282
// Read Helm chart YAML and apply templating
8383
let helm_file = helm_file.into_path_or_url().context(ParsePathOrUrlSnafu {
@@ -89,10 +89,7 @@ pub trait InstallManifestsExt {
8989
.await
9090
.context(FileTransferSnafu)?;
9191

92-
info!(
93-
"Installing Helm chart {} ({})",
94-
helm_chart.name, helm_chart.version
95-
);
92+
info!(helm_chart.name, helm_chart.version, "Installing Helm chart",);
9693

9794
// Assumption: that all manifest helm charts refer to repos not registries
9895
helm::add_repo(&helm_chart.repo.name, &helm_chart.repo.url).context(
@@ -122,7 +119,7 @@ pub trait InstallManifestsExt {
122119
})?;
123120
}
124121
ManifestSpec::PlainYaml(manifest_file) => {
125-
debug!("Installing YAML manifest from {}", manifest_file);
122+
debug!(manifest_file, "Installing YAML manifest");
126123

127124
// Read YAML manifest and apply templating
128125
let path_or_url =

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

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,13 +178,20 @@ impl OperatorSpec {
178178
}
179179

180180
/// Installs the operator using Helm.
181-
#[instrument(skip_all)]
181+
#[instrument(skip_all, fields(
182+
%namespace,
183+
name = %self.name,
184+
// NOTE (@NickLarsenNZ): Option doesn't impl Display, so we need to call
185+
// display for the inner type if it exists. Otherwise we gte the Debug
186+
// impl for the whole Option.
187+
version = self.version.as_ref().map(tracing::field::display),
188+
))]
182189
pub fn install(
183190
&self,
184191
namespace: &str,
185192
chart_source: &ChartSourceType,
186193
) -> Result<(), helm::Error> {
187-
info!("Installing operator {}", self);
194+
info!(operator = %self, "Installing operator");
188195

189196
let version = self.version.as_ref().map(|v| v.to_string());
190197
let helm_name = self.helm_name();
@@ -213,10 +220,10 @@ impl OperatorSpec {
213220
}
214221

215222
/// Uninstalls the operator using Helm.
216-
#[instrument]
223+
#[instrument(skip_all, fields(%namespace))]
217224
pub fn uninstall<T>(&self, namespace: T) -> Result<(), helm::Error>
218225
where
219-
T: AsRef<str> + std::fmt::Debug,
226+
T: AsRef<str> + std::fmt::Display + std::fmt::Debug,
220227
{
221228
match helm::uninstall_release(&self.helm_name(), namespace.as_ref(), true) {
222229
Ok(status) => {

‎rust/stackable-cockpit/src/platform/release/spec.rs

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ use indexmap::IndexMap;
33
use serde::{Deserialize, Serialize};
44
use snafu::{ResultExt, Snafu};
55
use tokio::task::JoinError;
6-
use tracing::{info, instrument};
6+
use tracing::{info, instrument, Instrument, Span};
77

88
#[cfg(feature = "openapi")]
99
use utoipa::ToSchema;
@@ -50,7 +50,11 @@ pub struct ReleaseSpec {
5050

5151
impl ReleaseSpec {
5252
/// Installs a release by installing individual operators.
53-
#[instrument(skip_all)]
53+
#[instrument(skip_all, fields(
54+
%namespace,
55+
product.included = tracing::field::Empty,
56+
product.excluded = tracing::field::Empty,
57+
))]
5458
pub async fn install(
5559
&self,
5660
include_products: &[String],
@@ -60,29 +64,44 @@ impl ReleaseSpec {
6064
) -> Result<()> {
6165
info!("Installing release");
6266

67+
include_products.iter().for_each(|product| {
68+
Span::current().record("product.included", product);
69+
});
70+
exclude_products.iter().for_each(|product| {
71+
Span::current().record("product.excluded", product);
72+
});
73+
6374
let namespace = namespace.to_string();
6475
futures::stream::iter(self.filter_products(include_products, exclude_products))
6576
.map(|(product_name, product)| {
77+
let task_span =
78+
tracing::debug_span!("install_operator", product_name = tracing::field::Empty);
79+
6680
let namespace = namespace.clone();
6781
let chart_source = chart_source.clone();
6882
// Helm installs currently `block_in_place`, so we need to spawn each job onto a separate task to
6983
// get useful parallelism.
70-
tokio::spawn(async move {
71-
info!("Installing {product_name}-operator");
72-
73-
// Create operator spec
74-
let operator = OperatorSpec::new(&product_name, Some(product.version.clone()))
75-
.context(OperatorSpecParseSnafu)?;
76-
77-
// Install operator
78-
operator
79-
.install(&namespace, &chart_source)
80-
.context(HelmInstallSnafu)?;
81-
82-
info!("Installed {product_name}-operator");
83-
84-
Ok(())
85-
})
84+
tokio::spawn(
85+
async move {
86+
Span::current().record("product_name", &product_name);
87+
info!("Installing {product_name}-operator");
88+
89+
// Create operator spec
90+
let operator =
91+
OperatorSpec::new(&product_name, Some(product.version.clone()))
92+
.context(OperatorSpecParseSnafu)?;
93+
94+
// Install operator
95+
operator
96+
.install(&namespace, &chart_source)
97+
.context(HelmInstallSnafu)?;
98+
99+
info!("Installed {product_name}-operator");
100+
101+
Ok(())
102+
}
103+
.instrument(task_span),
104+
)
86105
})
87106
.buffer_unordered(10)
88107
.map(|res| res.context(BackgroundTaskSnafu)?)

0 commit comments

Comments
 (0)