From c70bf809d8538eea3988489eab7a9de556647f72 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 6 Nov 2023 11:10:24 -0600 Subject: [PATCH 01/37] Add trace feature --- libcnb/Cargo.toml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libcnb/Cargo.toml b/libcnb/Cargo.toml index 9d50d9f8..34514983 100644 --- a/libcnb/Cargo.toml +++ b/libcnb/Cargo.toml @@ -14,12 +14,17 @@ include = ["src/**/*", "LICENSE", "README.md"] [lints] workspace = true +[features] +trace = ["dep:opentelemetry", "dep:opentelemetry-stdout"] + [dependencies] anyhow = { version = "1.0.75", optional = true } cyclonedx-bom = { version = "0.4.3", optional = true } libcnb-common.workspace = true libcnb-data.workspace = true libcnb-proc-macros.workspace = true +opentelemetry = { version = "0.20.0", optional = true } +opentelemetry-stdout = { version = "0.1.0", optional = true, features = ["trace"] } serde = { version = "1.0.192", features = ["derive"] } thiserror = "1.0.50" toml.workspace = true From 523603ce2a5ec07356b3514a9e0bab074d2a6704 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 11:10:40 -0600 Subject: [PATCH 02/37] Add tracing for detect --- libcnb/src/lib.rs | 2 + libcnb/src/runtime.rs | 30 ++++++++++++--- libcnb/src/tracing.rs | 88 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 5 deletions(-) create mode 100644 libcnb/src/tracing.rs diff --git a/libcnb/src/lib.rs b/libcnb/src/lib.rs index a65de479..05dc7e68 100644 --- a/libcnb/src/lib.rs +++ b/libcnb/src/lib.rs @@ -18,6 +18,8 @@ mod error; mod exit_code; mod platform; mod runtime; +#[cfg(feature = "trace")] +mod tracing; mod util; pub use buildpack::Buildpack; diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index d878b83e..aa666dc2 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -5,6 +5,8 @@ use crate::detect::{DetectContext, InnerDetectResult}; use crate::error::Error; use crate::platform::Platform; use crate::sbom::cnb_sbom_path; +#[cfg(feature = "trace")] +use crate::tracing::start_trace; use crate::util::is_not_found_error_kind; use crate::{exit_code, TomlFileError, LIBCNB_SUPPORTED_BUILDPACK_API}; use libcnb_common::toml_file::{read_toml_file, write_toml_file}; @@ -138,14 +140,32 @@ pub fn libcnb_runtime_detect( buildpack_descriptor: read_buildpack_descriptor()?, }; - match buildpack.detect(detect_context)?.0 { - InnerDetectResult::Fail => Ok(exit_code::DETECT_DETECTION_FAILED), + #[cfg(feature = "trace")] + let mut trace = start_trace(&detect_context.buildpack_descriptor.buildpack, "detect"); + + let detect_result = buildpack.detect(detect_context); + + #[cfg(feature = "trace")] + if let Err(ref err) = detect_result { + trace.set_error(err); + } + + match detect_result?.0 { + InnerDetectResult::Fail => { + #[cfg(feature = "trace")] + trace.add_event("detect-failed"); + Ok(exit_code::DETECT_DETECTION_FAILED) + } InnerDetectResult::Pass { build_plan } => { if let Some(build_plan) = build_plan { - write_toml_file(&build_plan, build_plan_path) - .map_err(Error::CannotWriteBuildPlan)?; + write_toml_file(&build_plan, build_plan_path).map_err(|err| { + #[cfg(feature = "trace")] + trace.set_error(&err); + Error::CannotWriteBuildPlan(err) + })?; } - + #[cfg(feature = "trace")] + trace.add_event("detect-passed"); Ok(exit_code::DETECT_DETECTION_PASSED) } } diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs new file mode 100644 index 00000000..5df4388d --- /dev/null +++ b/libcnb/src/tracing.rs @@ -0,0 +1,88 @@ +use libcnb_data::buildpack::Buildpack; +use opentelemetry::{ + global, + sdk::{ + trace::{Config, Span, TracerProvider}, + Resource, + }, + trace::{Span as SpanTrait, Status, Tracer, TracerProvider as TracerProviderTrait}, + KeyValue, +}; +use std::path::Path; + +pub(crate) struct BuildpackTrace { + provider: TracerProvider, + span: Span, +} + +pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { + let bp_slug = buildpack.id.replace(['/', '.'], "_"); + let tracing_file_path = Path::new("/tmp") + .join("cnb-telemetry") + .join(format!("{bp_slug}-{phase_name}.jsonl")); + + // Ensure tracing file path parent exists by creating it. + if let Some(parent_dir) = tracing_file_path.parent() { + let _ = std::fs::create_dir_all(parent_dir); + } + let exporter = match std::fs::File::options() + .create(true) + .append(true) + .open(&tracing_file_path) + { + Ok(file) => opentelemetry_stdout::SpanExporter::builder() + .with_writer(file) + .build(), + Err(_) => opentelemetry_stdout::SpanExporter::builder() + .with_writer(std::io::sink()) + .build(), + }; + + let lib_name = option_env!("CARGO_PKG_NAME").unwrap_or("libcnb"); + + let provider = opentelemetry::sdk::trace::TracerProvider::builder() + .with_simple_exporter(exporter) + .with_config( + Config::default() + .with_resource(Resource::new(vec![KeyValue::new("service.name", lib_name)])), + ) + .build(); + + global::set_tracer_provider(provider.clone()); + + let tracer = provider.versioned_tracer( + lib_name, + option_env!("CARGO_PKG_VERSION"), + None as Option<&str>, + None, + ); + + let mut span = tracer.start(phase_name); + span.set_attributes(vec![ + KeyValue::new("buildpack_id", buildpack.id.to_string().clone()), + KeyValue::new("buildpack_name", buildpack.name.clone().unwrap_or_default()), + KeyValue::new("buildpack_version", buildpack.version.to_string()), + KeyValue::new( + "buildpack_homepage", + buildpack.homepage.clone().unwrap_or_default(), + ), + ]); + BuildpackTrace { provider, span } +} + +impl BuildpackTrace { + pub(crate) fn set_error(&mut self, err: &dyn std::error::Error) { + self.span.set_status(Status::error(err.to_string())); + self.span.record_error(err); + } + pub(crate) fn add_event(&mut self, name: &'static str) { + self.span.add_event(name, vec![]); + } +} + +impl Drop for BuildpackTrace { + fn drop(&mut self) { + self.provider.force_flush(); + global::shutdown_tracer_provider(); + } +} From d68c36b7f15afecb612b23c090b78c21159dea87 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 11:50:34 -0600 Subject: [PATCH 03/37] Add tracing for buildpack build --- libcnb/src/runtime.rs | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index aa666dc2..b28f70d6 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -199,7 +199,7 @@ pub fn libcnb_runtime_build( } .map_err(Error::CannotReadStore)?; - let build_result = buildpack.build(BuildContext { + let build_context = BuildContext { layers_dir: layers_dir.clone(), app_dir, stack_id, @@ -208,9 +208,18 @@ pub fn libcnb_runtime_build( buildpack_dir: read_buildpack_dir()?, buildpack_descriptor: read_buildpack_descriptor()?, store, - })?; + }; + #[cfg(feature = "trace")] + let mut trace = start_trace(&build_context.buildpack_descriptor.buildpack, "build"); + + let build_result = buildpack.build(build_context); - match build_result.0 { + #[cfg(feature = "trace")] + if let Err(ref err) = build_result { + trace.set_error(err); + } + + match build_result?.0 { InnerBuildResult::Pass { launch, store, @@ -218,13 +227,21 @@ pub fn libcnb_runtime_build( launch_sboms, } => { if let Some(launch) = launch { - write_toml_file(&launch, layers_dir.join("launch.toml")) - .map_err(Error::CannotWriteLaunch)?; + write_toml_file(&launch, layers_dir.join("launch.toml")).map_err(|inner_err| { + let err = Error::CannotWriteLaunch(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; }; if let Some(store) = store { - write_toml_file(&store, layers_dir.join("store.toml")) - .map_err(Error::CannotWriteStore)?; + write_toml_file(&store, layers_dir.join("store.toml")).map_err(|inner_err| { + let err = Error::CannotWriteStore(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; }; for build_sbom in build_sboms { @@ -232,7 +249,12 @@ pub fn libcnb_runtime_build( cnb_sbom_path(&build_sbom.format, &layers_dir, "build"), &build_sbom.data, ) - .map_err(Error::CannotWriteBuildSbom)?; + .map_err(|inner_err| { + let err = Error::CannotWriteBuildSbom(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; } for launch_sbom in launch_sboms { @@ -243,6 +265,8 @@ pub fn libcnb_runtime_build( .map_err(Error::CannotWriteLaunchSbom)?; } + #[cfg(feature = "trace")] + trace.add_event("build-success"); Ok(exit_code::GENERIC_SUCCESS) } } From aae2e57561ad32c0bdf9263f0cd5384d96c79407 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 12:43:27 -0600 Subject: [PATCH 04/37] Add a tracing test --- libcnb/src/tracing.rs | 59 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 3 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 5df4388d..89e45f55 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -16,10 +16,10 @@ pub(crate) struct BuildpackTrace { } pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { - let bp_slug = buildpack.id.replace(['/', '.'], "_"); + let trace_name = format!("{}-{phase_name}", buildpack.id.replace(['/', '.'], "_")); let tracing_file_path = Path::new("/tmp") .join("cnb-telemetry") - .join(format!("{bp_slug}-{phase_name}.jsonl")); + .join(format!("{trace_name}.jsonl")); // Ensure tracing file path parent exists by creating it. if let Some(parent_dir) = tracing_file_path.parent() { @@ -57,7 +57,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu None, ); - let mut span = tracer.start(phase_name); + let mut span = tracer.start(trace_name); span.set_attributes(vec![ KeyValue::new("buildpack_id", buildpack.id.to_string().clone()), KeyValue::new("buildpack_name", buildpack.name.clone().unwrap_or_default()), @@ -82,7 +82,60 @@ impl BuildpackTrace { impl Drop for BuildpackTrace { fn drop(&mut self) { + self.span.end(); self.provider.force_flush(); global::shutdown_tracer_provider(); } } + +#[cfg(test)] +mod tests { + use super::start_trace; + use libcnb_data::buildpack::{Buildpack, BuildpackVersion}; + use std::{ + collections::HashSet, + fs, + io::{Error, ErrorKind}, + }; + + #[test] + fn test_tracing() { + let buildpack = Buildpack { + id: "company.com/foo" + .parse() + .expect("Valid BuildpackId should parse"), + version: BuildpackVersion::new(0, 0, 0), + name: Some("Foo buildpack for company.com".to_string()), + homepage: None, + clear_env: false, + description: None, + keywords: vec![], + licenses: vec![], + sbom_formats: HashSet::new(), + }; + let phase = "bar"; + let event = "baz-event"; + let error_message = "its broken"; + let telemetry_path = "/tmp/cnb-telemetry/company_com_foo-bar.jsonl"; + _ = fs::remove_file(telemetry_path); + + { + let mut trace = start_trace(&buildpack, &phase); + trace.add_event(event); + trace.set_error(&Error::new(ErrorKind::Other, error_message)); + } + let tracing_contents = fs::read_to_string(telemetry_path) + .expect("Expected telemetry file to exist, but couldn't read it"); + + println!("tracing_contents: {tracing_contents}"); + assert!(tracing_contents.contains(phase)); + assert!(tracing_contents.contains(event)); + assert!(tracing_contents.contains(error_message)); + assert!(tracing_contents.contains(buildpack.id.as_str())); + assert!(tracing_contents.contains(&buildpack.version.to_string())); + assert!( + tracing_contents.contains(&buildpack.name.expect("Expected buildpack.name to exist")) + ); + assert!(tracing_contents.contains("something")); + } +} From af371829f0558011ebca3ed781db821aa9055ca8 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 13:00:06 -0600 Subject: [PATCH 05/37] Add service.version to resource span --- libcnb/src/tracing.rs | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 89e45f55..591125aa 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -10,6 +10,8 @@ use opentelemetry::{ }; use std::path::Path; +const TELEMETRY_EXPORT_ROOT: &str = "/tmp/cnb-telemetry"; + pub(crate) struct BuildpackTrace { provider: TracerProvider, span: Span, @@ -17,9 +19,7 @@ pub(crate) struct BuildpackTrace { pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { let trace_name = format!("{}-{phase_name}", buildpack.id.replace(['/', '.'], "_")); - let tracing_file_path = Path::new("/tmp") - .join("cnb-telemetry") - .join(format!("{trace_name}.jsonl")); + let tracing_file_path = Path::new(TELEMETRY_EXPORT_ROOT).join(format!("{trace_name}.jsonl")); // Ensure tracing file path parent exists by creating it. if let Some(parent_dir) = tracing_file_path.parent() { @@ -39,23 +39,19 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu }; let lib_name = option_env!("CARGO_PKG_NAME").unwrap_or("libcnb"); + let lib_version = option_env!("CARGO_PKG_VERSION"); let provider = opentelemetry::sdk::trace::TracerProvider::builder() .with_simple_exporter(exporter) - .with_config( - Config::default() - .with_resource(Resource::new(vec![KeyValue::new("service.name", lib_name)])), - ) + .with_config(Config::default().with_resource(Resource::new(vec![ + KeyValue::new("service.name", lib_name), + KeyValue::new("service.version", lib_version.unwrap_or_default()), + ]))) .build(); global::set_tracer_provider(provider.clone()); - let tracer = provider.versioned_tracer( - lib_name, - option_env!("CARGO_PKG_VERSION"), - None as Option<&str>, - None, - ); + let tracer = provider.versioned_tracer(lib_name, lib_version, None as Option<&str>, None); let mut span = tracer.start(trace_name); span.set_attributes(vec![ From efb23254c24e7af742dc20c892116bc5bf4f358e Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 13:16:44 -0600 Subject: [PATCH 06/37] Cleanup clippy issue on newer rust --- libcnb/src/tracing.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 591125aa..e4c499c6 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -116,7 +116,7 @@ mod tests { _ = fs::remove_file(telemetry_path); { - let mut trace = start_trace(&buildpack, &phase); + let mut trace = start_trace(&buildpack, phase); trace.add_event(event); trace.set_error(&Error::new(ErrorKind::Other, error_message)); } @@ -132,6 +132,5 @@ mod tests { assert!( tracing_contents.contains(&buildpack.name.expect("Expected buildpack.name to exist")) ); - assert!(tracing_contents.contains("something")); } } From ac0fa8d989b19db8396b759de27f2650808e5325 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 8 Nov 2023 13:26:51 -0600 Subject: [PATCH 07/37] Add changelog entry for trace feature --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a4f728b..5bdc7558 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `libcnb`: + - An optional `trace` feature has been added that emits OpenTelemetry tracing + data to a [File Export](https://opentelemetry.io/docs/specs/otel/protocol/file-exporter/). ([#723](https://github.com/heroku/libcnb.rs/pull/723)) ## [0.16.0] - 2023-11-17 From 555839626b222f9b424cc8536c5f049df4e8e345 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Thu, 9 Nov 2023 08:53:57 -0600 Subject: [PATCH 08/37] Update to opentelemetry 0.21.0; use opentelemetry_sdk for sdk imports --- libcnb/Cargo.toml | 7 ++++--- libcnb/src/tracing.rs | 10 +++++----- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/libcnb/Cargo.toml b/libcnb/Cargo.toml index 34514983..b2dcf675 100644 --- a/libcnb/Cargo.toml +++ b/libcnb/Cargo.toml @@ -15,7 +15,7 @@ include = ["src/**/*", "LICENSE", "README.md"] workspace = true [features] -trace = ["dep:opentelemetry", "dep:opentelemetry-stdout"] +trace = ["dep:opentelemetry", "dep:opentelemetry_sdk", "dep:opentelemetry-stdout"] [dependencies] anyhow = { version = "1.0.75", optional = true } @@ -23,8 +23,9 @@ cyclonedx-bom = { version = "0.4.3", optional = true } libcnb-common.workspace = true libcnb-data.workspace = true libcnb-proc-macros.workspace = true -opentelemetry = { version = "0.20.0", optional = true } -opentelemetry-stdout = { version = "0.1.0", optional = true, features = ["trace"] } +opentelemetry = { version = "0.21.0", optional = true } +opentelemetry_sdk = { version = "0.21.0", optional = true } +opentelemetry-stdout = { version = "0.2.0", optional = true, features = ["trace"] } serde = { version = "1.0.192", features = ["derive"] } thiserror = "1.0.50" toml.workspace = true diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index e4c499c6..72f88981 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -1,13 +1,13 @@ use libcnb_data::buildpack::Buildpack; use opentelemetry::{ global, - sdk::{ - trace::{Config, Span, TracerProvider}, - Resource, - }, trace::{Span as SpanTrait, Status, Tracer, TracerProvider as TracerProviderTrait}, KeyValue, }; +use opentelemetry_sdk::{ + trace::{Config, Span, TracerProvider}, + Resource, +}; use std::path::Path; const TELEMETRY_EXPORT_ROOT: &str = "/tmp/cnb-telemetry"; @@ -41,7 +41,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu let lib_name = option_env!("CARGO_PKG_NAME").unwrap_or("libcnb"); let lib_version = option_env!("CARGO_PKG_VERSION"); - let provider = opentelemetry::sdk::trace::TracerProvider::builder() + let provider = TracerProvider::builder() .with_simple_exporter(exporter) .with_config(Config::default().with_resource(Resource::new(vec![ KeyValue::new("service.name", lib_name), From 33b2a99886fa5a3c4f55f7328a0bf6dc290e7a11 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Fri, 10 Nov 2023 10:06:10 -0600 Subject: [PATCH 09/37] Use the libcnb error rather than the io error when reporting to tracing --- libcnb/src/runtime.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index b28f70d6..ec55b42f 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -158,10 +158,11 @@ pub fn libcnb_runtime_detect( } InnerDetectResult::Pass { build_plan } => { if let Some(build_plan) = build_plan { - write_toml_file(&build_plan, build_plan_path).map_err(|err| { + write_toml_file(&build_plan, build_plan_path).map_err(|inner_err| { + let err = Error::CannotWriteBuildPlan(inner_err); #[cfg(feature = "trace")] trace.set_error(&err); - Error::CannotWriteBuildPlan(err) + err })?; } #[cfg(feature = "trace")] From adb88f9a906a45e17fe8defd5bb7eb3ee51040e7 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 15 Nov 2023 11:37:36 -0600 Subject: [PATCH 10/37] Add some documentation to tracing.rs --- libcnb/src/tracing.rs | 48 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 72f88981..a57a79e6 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -8,15 +8,28 @@ use opentelemetry_sdk::{ trace::{Config, Span, TracerProvider}, Resource, }; -use std::path::Path; +use std::{io::BufWriter, path::Path}; +// This is the diretory in which `BuildpackTrace` stores Open Telemetry File +// Exports. Services which intend to export the tracing data from libcnb.rs +// (such as [cnb-otel-collector](https://github.com/heroku/cnb-otel-collector)) +// should look for `.jsonl` file exports in this directory. This path was chosen +// to prevent conflicts with the CNB spec and /tmp is commonly available and +// writable on base images. +#[cfg(target_family = "unix")] const TELEMETRY_EXPORT_ROOT: &str = "/tmp/cnb-telemetry"; +/// `BuildpackTrace` represents an Open Telemetry tracer provider and single span. +/// It's designed to support tracing a CNB build or detect phase as a singular +/// span. pub(crate) struct BuildpackTrace { provider: TracerProvider, span: Span, } +/// `start_trace` starts an Open Telemetry trace and span that exports to an +/// Open Telemetry file export. The resulting trace provider and span are +/// enriched with data from the buildpack and the rust environment. pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { let trace_name = format!("{}-{phase_name}", buildpack.id.replace(['/', '.'], "_")); let tracing_file_path = Path::new(TELEMETRY_EXPORT_ROOT).join(format!("{trace_name}.jsonl")); @@ -30,28 +43,42 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu .append(true) .open(&tracing_file_path) { + // Write tracing data to a file, which may be read by other + // services. Wrap with a BufWriter to prevent serde from sending each + // JSON token to IO, and instead send entire JSON objects to IO. Ok(file) => opentelemetry_stdout::SpanExporter::builder() - .with_writer(file) + .with_writer(BufWriter::new(file)) .build(), + // Failed tracing shouldn't fail a build, and any logging here would + // likely confuse the user, so send telemetry to /dev/null on errors. Err(_) => opentelemetry_stdout::SpanExporter::builder() .with_writer(std::io::sink()) .build(), }; - let lib_name = option_env!("CARGO_PKG_NAME").unwrap_or("libcnb"); - let lib_version = option_env!("CARGO_PKG_VERSION"); - let provider = TracerProvider::builder() .with_simple_exporter(exporter) .with_config(Config::default().with_resource(Resource::new(vec![ - KeyValue::new("service.name", lib_name), - KeyValue::new("service.version", lib_version.unwrap_or_default()), + // Associate the tracer provider with service attributes. The buildpack + // name/version seems to map well to the suggestion + // [here](https://opentelemetry.io/docs/specs/semconv/resource/#service). + KeyValue::new("service.name", buildpack.id.to_string()), + KeyValue::new("service.version", buildpack.version.to_string()), ]))) .build(); + // Set the global tracer provider so that buildpacks may use it. global::set_tracer_provider(provider.clone()); - let tracer = provider.versioned_tracer(lib_name, lib_version, None as Option<&str>, None); + // Get a tracer identified by the instrumentation scope/library. The crate + // name/version seems to map well to the suggestion + // [here](https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer). + let tracer = provider.versioned_tracer( + option_env!("CARGO_PKG_NAME").unwrap_or("libcnb.rs"), + option_env!("CARGO_PKG_VERSION"), + None as Option<&str>, + None, + ); let mut span = tracer.start(trace_name); span.set_attributes(vec![ @@ -67,10 +94,13 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu } impl BuildpackTrace { + /// `set_error` sets the status for the underlying span to error, and + /// also records an exception on the span. pub(crate) fn set_error(&mut self, err: &dyn std::error::Error) { - self.span.set_status(Status::error(err.to_string())); + self.span.set_status(Status::error(format!("{err:?}"))); self.span.record_error(err); } + /// `add_event` adds a named event to the underlying span. pub(crate) fn add_event(&mut self, name: &'static str) { self.span.add_event(name, vec![]); } From 2a9cae5e0cbef83570b8c3f694a17c716e8294b6 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Wed, 15 Nov 2023 11:57:03 -0600 Subject: [PATCH 11/37] Move telemetry path to libcnb --- libcnb/src/tracing.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index a57a79e6..ae76df7c 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -17,7 +17,7 @@ use std::{io::BufWriter, path::Path}; // to prevent conflicts with the CNB spec and /tmp is commonly available and // writable on base images. #[cfg(target_family = "unix")] -const TELEMETRY_EXPORT_ROOT: &str = "/tmp/cnb-telemetry"; +const TELEMETRY_EXPORT_ROOT: &str = "/tmp/libcnb-telemetry"; /// `BuildpackTrace` represents an Open Telemetry tracer provider and single span. /// It's designed to support tracing a CNB build or detect phase as a singular @@ -142,7 +142,7 @@ mod tests { let phase = "bar"; let event = "baz-event"; let error_message = "its broken"; - let telemetry_path = "/tmp/cnb-telemetry/company_com_foo-bar.jsonl"; + let telemetry_path = "/tmp/libcnb-telemetry/company_com_foo-bar.jsonl"; _ = fs::remove_file(telemetry_path); { From 3f5cb51fbe2837e5b49d64221a2164413c69ad7f Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 28 Nov 2023 15:28:59 -0600 Subject: [PATCH 12/37] Add tracing integration test --- Cargo.toml | 1 + test-buildpacks/tracing/Cargo.toml | 12 +++++++ test-buildpacks/tracing/buildpack.toml | 9 ++++++ test-buildpacks/tracing/src/main.rs | 28 ++++++++++++++++ .../buildpacks/tracing-reader/bin/build | 13 ++++++++ .../buildpacks/tracing-reader/bin/detect | 2 ++ .../buildpacks/tracing-reader/buildpack.toml | 9 ++++++ .../tracing/tests/integration_test.rs | 32 +++++++++++++++++++ 8 files changed, 106 insertions(+) create mode 100644 test-buildpacks/tracing/Cargo.toml create mode 100644 test-buildpacks/tracing/buildpack.toml create mode 100644 test-buildpacks/tracing/src/main.rs create mode 100755 test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build create mode 100755 test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect create mode 100644 test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/buildpack.toml create mode 100644 test-buildpacks/tracing/tests/integration_test.rs diff --git a/Cargo.toml b/Cargo.toml index fed493e9..562a1e05 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ members = [ "test-buildpacks/readonly-layer-files", "test-buildpacks/sbom", "test-buildpacks/store", + "test-buildpacks/tracing", ] [workspace.package] diff --git a/test-buildpacks/tracing/Cargo.toml b/test-buildpacks/tracing/Cargo.toml new file mode 100644 index 00000000..6262005b --- /dev/null +++ b/test-buildpacks/tracing/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "tracing" +version = "0.0.0" +edition.workspace = true +rust-version.workspace = true +publish = false + +[dependencies] +libcnb = { path = "../../libcnb", features = ["trace"] } + +[dev-dependencies] +libcnb-test.workspace = true diff --git a/test-buildpacks/tracing/buildpack.toml b/test-buildpacks/tracing/buildpack.toml new file mode 100644 index 00000000..3820284a --- /dev/null +++ b/test-buildpacks/tracing/buildpack.toml @@ -0,0 +1,9 @@ +api = "0.9" + +[buildpack] +id = "libcnb-test-buildpacks/tracing" +version = "0.1.0" +name = "libcnb test buildpack: tracing" + +[[stacks]] +id = "*" diff --git a/test-buildpacks/tracing/src/main.rs b/test-buildpacks/tracing/src/main.rs new file mode 100644 index 00000000..6e7f47c2 --- /dev/null +++ b/test-buildpacks/tracing/src/main.rs @@ -0,0 +1,28 @@ +#![warn(clippy::pedantic)] +#![allow(clippy::module_name_repetitions)] + +use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder}; +use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder}; +use libcnb::generic::{GenericError, GenericMetadata, GenericPlatform}; +use libcnb::{buildpack_main, Buildpack}; + +/// TestTracingBuildpack is a basic buildpack compiled with the libcnb.rs +/// `tracing` flag, which should emit opentelemetry file exports to the build +/// file system (but not the final image). +pub struct TestTracingBuildpack; + +impl Buildpack for TestTracingBuildpack { + type Platform = GenericPlatform; + type Metadata = GenericMetadata; + type Error = GenericError; + + fn detect(&self, _context: DetectContext) -> libcnb::Result { + DetectResultBuilder::pass().build() + } + + fn build(&self, _context: BuildContext) -> libcnb::Result { + BuildResultBuilder::new().build() + } +} + +buildpack_main!(TestTracingBuildpack); diff --git a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build new file mode 100755 index 00000000..c2f5004b --- /dev/null +++ b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build @@ -0,0 +1,13 @@ +#!/usr/bin/env bash +set -eo pipefail + +echo "---> Tracing Reader Buildpack" + +# Report the contents of previous buildpack tracing file exports. +# Useful for testing the contents of tracing file contents, which aren't +# available in the resulting image of a CNB build. +for tracing_file in /tmp/libcnb-telemetry/*.jsonl; do + cat $tracing_file +done + +exit 0 diff --git a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect new file mode 100755 index 00000000..45111a15 --- /dev/null +++ b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect @@ -0,0 +1,2 @@ +#!/usr/bin/env bash +set -eo pipefail diff --git a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/buildpack.toml b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/buildpack.toml new file mode 100644 index 00000000..3b35a264 --- /dev/null +++ b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/buildpack.toml @@ -0,0 +1,9 @@ +api = "0.9" + +[buildpack] +id = "libcnb-test-buildpacks/tracing-reader" +version = "0.1.0" +name = "libcnb test buildpack fixture: tracing-reader" + +[[stacks]] +id = "*" diff --git a/test-buildpacks/tracing/tests/integration_test.rs b/test-buildpacks/tracing/tests/integration_test.rs new file mode 100644 index 00000000..e49e2905 --- /dev/null +++ b/test-buildpacks/tracing/tests/integration_test.rs @@ -0,0 +1,32 @@ +#![warn(clippy::pedantic)] + +use libcnb_test::{assert_contains, BuildConfig, BuildpackReference, TestRunner}; +use std::env::temp_dir; +use std::fs; + +#[test] +#[ignore = "integration test"] +fn test_tracing_export_file() { + let empty_app_dir = temp_dir().join("empty-app-dir"); + fs::create_dir_all(&empty_app_dir).unwrap(); + + let mut build_config = BuildConfig::new("heroku/builder:22", &empty_app_dir); + + // Telemetry file exports are not persisted to the build's resulting image, + // so to test that contents are emitted, a second buildpack is used to read + // the contents during the build. + build_config.buildpacks(vec![ + BuildpackReference::CurrentCrate, + BuildpackReference::Other(format!( + "file://{}/tests/fixtures/buildpacks/tracing-reader", + env!("CARGO_MANIFEST_DIR") + )), + ]); + + TestRunner::default().build(&build_config, |context| { + // Ensure expected span names for detect and build phases are present + // in the file export contents. + assert_contains!(context.pack_stdout, "libcnb-test-buildpacks_tracing-detect"); + assert_contains!(context.pack_stdout, "libcnb-test-buildpacks_tracing-build"); + }); +} From 4edd3071cde877d960175a53a10bc9e225a89081 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 28 Nov 2023 16:47:10 -0600 Subject: [PATCH 13/37] Verify that tracing data is valid JSON --- libcnb/Cargo.toml | 1 + libcnb/src/tracing.rs | 3 +++ 2 files changed, 4 insertions(+) diff --git a/libcnb/Cargo.toml b/libcnb/Cargo.toml index b2dcf675..2394b8fe 100644 --- a/libcnb/Cargo.toml +++ b/libcnb/Cargo.toml @@ -33,3 +33,4 @@ toml.workspace = true [dev-dependencies] fastrand = "2.0.1" tempfile = "3.8.1" +serde_json = "1.0.108" diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index ae76df7c..6c53e82a 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -118,6 +118,7 @@ impl Drop for BuildpackTrace { mod tests { use super::start_trace; use libcnb_data::buildpack::{Buildpack, BuildpackVersion}; + use serde_json::Value; use std::{ collections::HashSet, fs, @@ -154,6 +155,8 @@ mod tests { .expect("Expected telemetry file to exist, but couldn't read it"); println!("tracing_contents: {tracing_contents}"); + let _tracing_data: Value = serde_json::from_str(&tracing_contents) + .expect("Expected tracing export file contents to be valid json"); assert!(tracing_contents.contains(phase)); assert!(tracing_contents.contains(event)); assert!(tracing_contents.contains(error_message)); From 5b911dbb76370d84e711a35e76588778dae3c714 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 28 Nov 2023 16:54:20 -0600 Subject: [PATCH 14/37] Additionally transform hyphens to underscores for trace names --- libcnb/src/tracing.rs | 5 ++++- test-buildpacks/tracing/tests/integration_test.rs | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 6c53e82a..3900791a 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -31,7 +31,10 @@ pub(crate) struct BuildpackTrace { /// Open Telemetry file export. The resulting trace provider and span are /// enriched with data from the buildpack and the rust environment. pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { - let trace_name = format!("{}-{phase_name}", buildpack.id.replace(['/', '.'], "_")); + let trace_name = format!( + "{}-{phase_name}", + buildpack.id.replace(['/', '.', '-'], "_") + ); let tracing_file_path = Path::new(TELEMETRY_EXPORT_ROOT).join(format!("{trace_name}.jsonl")); // Ensure tracing file path parent exists by creating it. diff --git a/test-buildpacks/tracing/tests/integration_test.rs b/test-buildpacks/tracing/tests/integration_test.rs index e49e2905..177d124a 100644 --- a/test-buildpacks/tracing/tests/integration_test.rs +++ b/test-buildpacks/tracing/tests/integration_test.rs @@ -26,7 +26,7 @@ fn test_tracing_export_file() { TestRunner::default().build(&build_config, |context| { // Ensure expected span names for detect and build phases are present // in the file export contents. - assert_contains!(context.pack_stdout, "libcnb-test-buildpacks_tracing-detect"); - assert_contains!(context.pack_stdout, "libcnb-test-buildpacks_tracing-build"); + assert_contains!(context.pack_stdout, "libcnb_test_buildpacks_tracing-detect"); + assert_contains!(context.pack_stdout, "libcnb_test_buildpacks_tracing-build"); }); } From c43b217dd7ca63c1fdc43d1796a9525b5a7e2dfb Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Thu, 30 Nov 2023 15:57:24 -0600 Subject: [PATCH 15/37] Fix spelling typo --- libcnb/src/tracing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 3900791a..c09bf82a 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -10,7 +10,7 @@ use opentelemetry_sdk::{ }; use std::{io::BufWriter, path::Path}; -// This is the diretory in which `BuildpackTrace` stores Open Telemetry File +// This is the directory in which `BuildpackTrace` stores Open Telemetry File // Exports. Services which intend to export the tracing data from libcnb.rs // (such as [cnb-otel-collector](https://github.com/heroku/cnb-otel-collector)) // should look for `.jsonl` file exports in this directory. This path was chosen From 982ca526e3f1ea67c6475e683df094e490bcd62f Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Thu, 30 Nov 2023 16:01:25 -0600 Subject: [PATCH 16/37] Slight update to test buildpack documentation --- test-buildpacks/tracing/src/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test-buildpacks/tracing/src/main.rs b/test-buildpacks/tracing/src/main.rs index 6e7f47c2..90b6842b 100644 --- a/test-buildpacks/tracing/src/main.rs +++ b/test-buildpacks/tracing/src/main.rs @@ -6,8 +6,8 @@ use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder}; use libcnb::generic::{GenericError, GenericMetadata, GenericPlatform}; use libcnb::{buildpack_main, Buildpack}; -/// TestTracingBuildpack is a basic buildpack compiled with the libcnb.rs -/// `tracing` flag, which should emit opentelemetry file exports to the build +/// `TestTracingBuildpack` is a basic buildpack compiled with the libcnb.rs +/// `trace` flag, which should emit opentelemetry file exports to the build /// file system (but not the final image). pub struct TestTracingBuildpack; From d829c562b04ff90e3ebd375ae7face79b31aba6a Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 10:43:59 -0600 Subject: [PATCH 17/37] Use workspace lints in test buildpack Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- test-buildpacks/tracing/Cargo.toml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test-buildpacks/tracing/Cargo.toml b/test-buildpacks/tracing/Cargo.toml index 6262005b..25d08d51 100644 --- a/test-buildpacks/tracing/Cargo.toml +++ b/test-buildpacks/tracing/Cargo.toml @@ -5,6 +5,9 @@ edition.workspace = true rust-version.workspace = true publish = false +[lints] +workspace = true + [dependencies] libcnb = { path = "../../libcnb", features = ["trace"] } From 8cdb90412a049e0fa6db4a5810cd5aa6d3a7fdda Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 10:44:20 -0600 Subject: [PATCH 18/37] Prefer workspace lints Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- test-buildpacks/tracing/src/main.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/test-buildpacks/tracing/src/main.rs b/test-buildpacks/tracing/src/main.rs index 90b6842b..7663e5f1 100644 --- a/test-buildpacks/tracing/src/main.rs +++ b/test-buildpacks/tracing/src/main.rs @@ -1,6 +1,3 @@ -#![warn(clippy::pedantic)] -#![allow(clippy::module_name_repetitions)] - use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder}; use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder}; use libcnb::generic::{GenericError, GenericMetadata, GenericPlatform}; From 02cadc8fe42d6c68eb5316afe475d658358946e1 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 10:54:02 -0600 Subject: [PATCH 19/37] Prefer workspace lints Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- test-buildpacks/tracing/tests/integration_test.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/test-buildpacks/tracing/tests/integration_test.rs b/test-buildpacks/tracing/tests/integration_test.rs index 177d124a..83f2486d 100644 --- a/test-buildpacks/tracing/tests/integration_test.rs +++ b/test-buildpacks/tracing/tests/integration_test.rs @@ -1,5 +1,3 @@ -#![warn(clippy::pedantic)] - use libcnb_test::{assert_contains, BuildConfig, BuildpackReference, TestRunner}; use std::env::temp_dir; use std::fs; From 71c3ba351d4f312284896cfe4e9e6147ef7e094a Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 10:55:22 -0600 Subject: [PATCH 20/37] Use workspace dependency Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- test-buildpacks/tracing/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-buildpacks/tracing/Cargo.toml b/test-buildpacks/tracing/Cargo.toml index 25d08d51..cd7edb81 100644 --- a/test-buildpacks/tracing/Cargo.toml +++ b/test-buildpacks/tracing/Cargo.toml @@ -9,7 +9,7 @@ publish = false workspace = true [dependencies] -libcnb = { path = "../../libcnb", features = ["trace"] } +libcnb = { workspace = true, features = ["trace"] } [dev-dependencies] libcnb-test.workspace = true From 8a7c4c9c16cdb01967cb6d62ee4cb03350d96c0e Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 10:59:59 -0600 Subject: [PATCH 21/37] Be specific about which crate Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- libcnb/src/tracing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index c09bf82a..6ac497a0 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -73,7 +73,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu // Set the global tracer provider so that buildpacks may use it. global::set_tracer_provider(provider.clone()); - // Get a tracer identified by the instrumentation scope/library. The crate + // Get a tracer identified by the instrumentation scope/library. The libcnb crate // name/version seems to map well to the suggestion // [here](https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer). let tracer = provider.versioned_tracer( From 56ee596de41085585522f23c5d15fca8e486d8f6 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:02:57 -0600 Subject: [PATCH 22/37] Use an array over a vector Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- test-buildpacks/tracing/tests/integration_test.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-buildpacks/tracing/tests/integration_test.rs b/test-buildpacks/tracing/tests/integration_test.rs index 83f2486d..76f52e48 100644 --- a/test-buildpacks/tracing/tests/integration_test.rs +++ b/test-buildpacks/tracing/tests/integration_test.rs @@ -13,7 +13,7 @@ fn test_tracing_export_file() { // Telemetry file exports are not persisted to the build's resulting image, // so to test that contents are emitted, a second buildpack is used to read // the contents during the build. - build_config.buildpacks(vec![ + build_config.buildpacks([ BuildpackReference::CurrentCrate, BuildpackReference::Other(format!( "file://{}/tests/fixtures/buildpacks/tracing-reader", From 3664926087fb46e4b2d036ddd0b30ab29d138fe4 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:03:17 -0600 Subject: [PATCH 23/37] Drop redundant clone Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> --- libcnb/src/tracing.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 6ac497a0..3add0ccd 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -85,7 +85,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu let mut span = tracer.start(trace_name); span.set_attributes(vec![ - KeyValue::new("buildpack_id", buildpack.id.to_string().clone()), + KeyValue::new("buildpack_id", buildpack.id.to_string()), KeyValue::new("buildpack_name", buildpack.name.clone().unwrap_or_default()), KeyValue::new("buildpack_version", buildpack.version.to_string()), KeyValue::new( From c24241566fd016c905d7ab28af9ce052ebd6ce4a Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:11:32 -0600 Subject: [PATCH 24/37] Silence unused crate warning for serde_json --- libcnb/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libcnb/src/lib.rs b/libcnb/src/lib.rs index 05dc7e68..1c3fa7ac 100644 --- a/libcnb/src/lib.rs +++ b/libcnb/src/lib.rs @@ -29,6 +29,9 @@ pub use libcnb_common::toml_file::*; pub use platform::*; pub use runtime::*; +#[cfg(all(test, not(feature = "trace")))] +use serde_json as _; + /// Provides types for CNB data formats. Is a re-export of the `libcnb-data` crate. #[doc(inline)] pub use libcnb_data as data; From 0c18d964ad0c9103ee3d5969da998326503ba3d9 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:23:11 -0600 Subject: [PATCH 25/37] Update tracing documentation and comments --- libcnb/src/tracing.rs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 3add0ccd..7734514d 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -10,25 +10,24 @@ use opentelemetry_sdk::{ }; use std::{io::BufWriter, path::Path}; -// This is the directory in which `BuildpackTrace` stores Open Telemetry File +// This is the directory in which `BuildpackTrace` stores OpenTelemetry File // Exports. Services which intend to export the tracing data from libcnb.rs -// (such as [cnb-otel-collector](https://github.com/heroku/cnb-otel-collector)) +// (such as https://github.com/heroku/cnb-otel-collector) // should look for `.jsonl` file exports in this directory. This path was chosen // to prevent conflicts with the CNB spec and /tmp is commonly available and // writable on base images. #[cfg(target_family = "unix")] const TELEMETRY_EXPORT_ROOT: &str = "/tmp/libcnb-telemetry"; -/// `BuildpackTrace` represents an Open Telemetry tracer provider and single span. -/// It's designed to support tracing a CNB build or detect phase as a singular -/// span. +/// Represents an OpenTelemetry tracer provider and single span tracing +/// a single CNB build or detect phase. pub(crate) struct BuildpackTrace { provider: TracerProvider, span: Span, } -/// `start_trace` starts an Open Telemetry trace and span that exports to an -/// Open Telemetry file export. The resulting trace provider and span are +/// Start an OpenTelemetry trace and span that exports to an +/// OpenTelemetry file export. The resulting trace provider and span are /// enriched with data from the buildpack and the rust environment. pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> BuildpackTrace { let trace_name = format!( @@ -63,8 +62,8 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu .with_simple_exporter(exporter) .with_config(Config::default().with_resource(Resource::new(vec![ // Associate the tracer provider with service attributes. The buildpack - // name/version seems to map well to the suggestion - // [here](https://opentelemetry.io/docs/specs/semconv/resource/#service). + // name/version seems to map well to the suggestion here + // https://opentelemetry.io/docs/specs/semconv/resource/#service. KeyValue::new("service.name", buildpack.id.to_string()), KeyValue::new("service.version", buildpack.version.to_string()), ]))) @@ -74,8 +73,8 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu global::set_tracer_provider(provider.clone()); // Get a tracer identified by the instrumentation scope/library. The libcnb crate - // name/version seems to map well to the suggestion - // [here](https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer). + // name/version seems to map well to the suggestion here: + // https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer. let tracer = provider.versioned_tracer( option_env!("CARGO_PKG_NAME").unwrap_or("libcnb.rs"), option_env!("CARGO_PKG_VERSION"), @@ -97,13 +96,13 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu } impl BuildpackTrace { - /// `set_error` sets the status for the underlying span to error, and - /// also records an exception on the span. + /// Set the status for the underlying span to error, and record + /// an exception on the span. pub(crate) fn set_error(&mut self, err: &dyn std::error::Error) { self.span.set_status(Status::error(format!("{err:?}"))); self.span.record_error(err); } - /// `add_event` adds a named event to the underlying span. + /// Add a named event to the underlying span. pub(crate) fn add_event(&mut self, name: &'static str) { self.span.add_event(name, vec![]); } @@ -120,7 +119,10 @@ impl Drop for BuildpackTrace { #[cfg(test)] mod tests { use super::start_trace; - use libcnb_data::buildpack::{Buildpack, BuildpackVersion}; + use libcnb_data::{ + buildpack::{Buildpack, BuildpackVersion}, + buildpack_id, + }; use serde_json::Value; use std::{ collections::HashSet, @@ -131,9 +133,7 @@ mod tests { #[test] fn test_tracing() { let buildpack = Buildpack { - id: "company.com/foo" - .parse() - .expect("Valid BuildpackId should parse"), + id: buildpack_id!("company.com/foo"), version: BuildpackVersion::new(0, 0, 0), name: Some("Foo buildpack for company.com".to_string()), homepage: None, From 690e0d7074597e66561cd828f4f18e73007520b3 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:36:28 -0600 Subject: [PATCH 26/37] Use set_error in when failing to write sboms --- libcnb/src/runtime.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index ec55b42f..7655c31a 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -263,7 +263,12 @@ pub fn libcnb_runtime_build( cnb_sbom_path(&launch_sbom.format, &layers_dir, "launch"), &launch_sbom.data, ) - .map_err(Error::CannotWriteLaunchSbom)?; + .map_err(|inner_err| { + let err = Error::CannotWriteLaunchSbom(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; } #[cfg(feature = "trace")] From 1c6b7c77f8d1f136955a58ae147e310ed6cdaafc Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 11:41:06 -0600 Subject: [PATCH 27/37] Fail if CARGO_PKG_NAME is unset for tracing --- libcnb/src/tracing.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 7734514d..f88ff7e8 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -72,12 +72,12 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu // Set the global tracer provider so that buildpacks may use it. global::set_tracer_provider(provider.clone()); - // Get a tracer identified by the instrumentation scope/library. The libcnb crate - // name/version seems to map well to the suggestion here: + // Get a tracer identified by the instrumentation scope/library. The libcnb + // crate name/version seems to map well to the suggestion here: // https://opentelemetry.io/docs/specs/otel/trace/api/#get-a-tracer. let tracer = provider.versioned_tracer( - option_env!("CARGO_PKG_NAME").unwrap_or("libcnb.rs"), - option_env!("CARGO_PKG_VERSION"), + env!("CARGO_PKG_NAME"), + Some(env!("CARGO_PKG_VERSION")), None as Option<&str>, None, ); From f6486ea3c516a7e91efea5928189ca3c215f37d9 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 20:12:46 -0600 Subject: [PATCH 28/37] Set error on traces in more error scenarios --- libcnb/src/runtime.rs | 79 ++++++++++++++++++++++++++++++++----------- 1 file changed, 59 insertions(+), 20 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 7655c31a..27cdc465 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -10,6 +10,7 @@ use crate::tracing::start_trace; use crate::util::is_not_found_error_kind; use crate::{exit_code, TomlFileError, LIBCNB_SUPPORTED_BUILDPACK_API}; use libcnb_common::toml_file::{read_toml_file, write_toml_file}; +use libcnb_data::buildpack::ComponentBuildpackDescriptor; use libcnb_data::store::Store; use serde::de::DeserializeOwned; use serde::Deserialize; @@ -123,12 +124,29 @@ pub fn libcnb_runtime_detect( ) -> crate::Result { let app_dir = env::current_dir().map_err(Error::CannotDetermineAppDirectory)?; + let buildpack_dir = read_buildpack_dir()?; + + let buildpack_descriptor: ComponentBuildpackDescriptor<::Metadata> = + read_buildpack_descriptor()?; + + #[cfg(feature = "trace")] + let mut trace = start_trace(&buildpack_descriptor.buildpack, "detect"); + let stack_id: StackId = env::var("CNB_STACK_ID") .map_err(Error::CannotDetermineStackId) - .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError))?; + .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) + .map_err(|err| { + #[cfg(feature = "trace")] + trace.set_error(err); + err + })?; - let platform = B::Platform::from_path(&args.platform_dir_path) - .map_err(Error::CannotCreatePlatformFromPath)?; + let platform = B::Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { + let err = Error::CannotCreatePlatformFromPath(inner_err); + #[cfg(feature = "trace")] + trace.set_error(err); + err + })?; let build_plan_path = args.build_plan_path; @@ -136,13 +154,10 @@ pub fn libcnb_runtime_detect( app_dir, stack_id, platform, - buildpack_dir: read_buildpack_dir()?, - buildpack_descriptor: read_buildpack_descriptor()?, + buildpack_dir, + buildpack_descriptor, }; - #[cfg(feature = "trace")] - let mut trace = start_trace(&detect_context.buildpack_descriptor.buildpack, "detect"); - let detect_result = buildpack.detect(detect_context); #[cfg(feature = "trace")] @@ -184,21 +199,47 @@ pub fn libcnb_runtime_build( let app_dir = env::current_dir().map_err(Error::CannotDetermineAppDirectory)?; - let stack_id: StackId = env::var("CNB_STACK_ID") - .map_err(Error::CannotDetermineStackId) - .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError))?; + let buildpack_dir = read_buildpack_dir()?; - let platform = Platform::from_path(&args.platform_dir_path) - .map_err(Error::CannotCreatePlatformFromPath)?; + let buildpack_descriptor: ComponentBuildpackDescriptor<::Metadata> = + read_buildpack_descriptor()?; + + #[cfg(feature = "trace")] + let mut trace = start_trace(&buildpack_descriptor.buildpack, "build"); - let buildpack_plan = - read_toml_file(&args.buildpack_plan_path).map_err(Error::CannotReadBuildpackPlan)?; + let stack_id: StackId = env::var("CNB_STACK_ID") + .map_err(Error::CannotDetermineStackId) + .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) + .map_err(|err| { + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; + + let platform = Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { + let err = Error::CannotCreatePlatformFromPath(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; + + let buildpack_plan = read_toml_file(&args.buildpack_plan_path).map_err(|inner_err| { + let err = Error::CannotReadBuildpackPlan(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; let store = match read_toml_file::(layers_dir.join("store.toml")) { Err(TomlFileError::IoError(io_error)) if is_not_found_error_kind(&io_error) => Ok(None), other => other.map(Some), } - .map_err(Error::CannotReadStore)?; + .map_err(|inner_err| { + let err = Error::CannotReadStore(inner_err); + #[cfg(feature = "trace")] + trace.set_error(&err); + err + })?; let build_context = BuildContext { layers_dir: layers_dir.clone(), @@ -206,12 +247,10 @@ pub fn libcnb_runtime_build( stack_id, platform, buildpack_plan, - buildpack_dir: read_buildpack_dir()?, - buildpack_descriptor: read_buildpack_descriptor()?, + buildpack_dir, + buildpack_descriptor, store, }; - #[cfg(feature = "trace")] - let mut trace = start_trace(&build_context.buildpack_descriptor.buildpack, "build"); let build_result = buildpack.build(build_context); From 3f641b072a5817c2d2be2fb03ef64c00656da8c6 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 20:17:51 -0600 Subject: [PATCH 29/37] Drop unnecessary vec allocations --- libcnb/src/runtime.rs | 4 ++-- libcnb/src/tracing.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 27cdc465..42db7072 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -137,14 +137,14 @@ pub fn libcnb_runtime_detect( .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) .map_err(|err| { #[cfg(feature = "trace")] - trace.set_error(err); + trace.set_error(&err); err })?; let platform = B::Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { let err = Error::CannotCreatePlatformFromPath(inner_err); #[cfg(feature = "trace")] - trace.set_error(err); + trace.set_error(&err); err })?; diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index f88ff7e8..ef71037b 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -60,7 +60,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu let provider = TracerProvider::builder() .with_simple_exporter(exporter) - .with_config(Config::default().with_resource(Resource::new(vec![ + .with_config(Config::default().with_resource(Resource::new([ // Associate the tracer provider with service attributes. The buildpack // name/version seems to map well to the suggestion here // https://opentelemetry.io/docs/specs/semconv/resource/#service. @@ -83,7 +83,7 @@ pub(crate) fn start_trace(buildpack: &Buildpack, phase_name: &'static str) -> Bu ); let mut span = tracer.start(trace_name); - span.set_attributes(vec![ + span.set_attributes([ KeyValue::new("buildpack_id", buildpack.id.to_string()), KeyValue::new("buildpack_name", buildpack.name.clone().unwrap_or_default()), KeyValue::new("buildpack_version", buildpack.version.to_string()), From ff02453b0dfd1f3d56aa4517422c758ac173343e Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Mon, 4 Dec 2023 20:38:25 -0600 Subject: [PATCH 30/37] Fix a few clippy lints --- libcnb/src/runtime.rs | 2 +- libcnb/src/tracing.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 42db7072..11095ac9 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -152,9 +152,9 @@ pub fn libcnb_runtime_detect( let detect_context = DetectContext { app_dir, + buildpack_dir, stack_id, platform, - buildpack_dir, buildpack_descriptor, }; diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index ef71037b..61c40078 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -1,3 +1,4 @@ +#![allow(clippy::doc_markdown)] use libcnb_data::buildpack::Buildpack; use opentelemetry::{ global, From f4b440e7e09202dc814dc9bceab72094ed3adf6b Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 08:31:42 -0600 Subject: [PATCH 31/37] Silence a few more clippy lints --- test-buildpacks/tracing/src/main.rs | 4 ++++ test-buildpacks/tracing/tests/integration_test.rs | 3 +++ 2 files changed, 7 insertions(+) diff --git a/test-buildpacks/tracing/src/main.rs b/test-buildpacks/tracing/src/main.rs index 7663e5f1..85b0cdb4 100644 --- a/test-buildpacks/tracing/src/main.rs +++ b/test-buildpacks/tracing/src/main.rs @@ -3,6 +3,10 @@ use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder}; use libcnb::generic::{GenericError, GenericMetadata, GenericPlatform}; use libcnb::{buildpack_main, Buildpack}; +// Suppress warnings due to the `unused_crate_dependencies` lint not handling integration tests well. +#[cfg(test)] +use libcnb_test as _; + /// `TestTracingBuildpack` is a basic buildpack compiled with the libcnb.rs /// `trace` flag, which should emit opentelemetry file exports to the build /// file system (but not the final image). diff --git a/test-buildpacks/tracing/tests/integration_test.rs b/test-buildpacks/tracing/tests/integration_test.rs index 76f52e48..4ad6cbf7 100644 --- a/test-buildpacks/tracing/tests/integration_test.rs +++ b/test-buildpacks/tracing/tests/integration_test.rs @@ -1,3 +1,6 @@ +// Required due to: https://github.com/rust-lang/rust/issues/95513 +#![allow(unused_crate_dependencies)] + use libcnb_test::{assert_contains, BuildConfig, BuildpackReference, TestRunner}; use std::env::temp_dir; use std::fs; From 6bf7261590d7251ee145135de5710b8efa9b5c56 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 11:43:13 -0600 Subject: [PATCH 32/37] Clean up runtime repeated use of cfg feature check --- libcnb/src/runtime.rs | 74 ++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 11095ac9..026108e0 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -132,19 +132,21 @@ pub fn libcnb_runtime_detect( #[cfg(feature = "trace")] let mut trace = start_trace(&buildpack_descriptor.buildpack, "detect"); + let mut record_trace_err = |err: &dyn std::error::Error| { + #[cfg(feature = "trace")] + trace.set_error(err); + }; let stack_id: StackId = env::var("CNB_STACK_ID") .map_err(Error::CannotDetermineStackId) .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) .map_err(|err| { - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; let platform = B::Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { let err = Error::CannotCreatePlatformFromPath(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; @@ -158,12 +160,10 @@ pub fn libcnb_runtime_detect( buildpack_descriptor, }; - let detect_result = buildpack.detect(detect_context); - - #[cfg(feature = "trace")] - if let Err(ref err) = detect_result { - trace.set_error(err); - } + let detect_result = buildpack.detect(detect_context).map_err(|err| { + record_trace_err(&err); + err + }); match detect_result?.0 { InnerDetectResult::Fail => { @@ -175,8 +175,7 @@ pub fn libcnb_runtime_detect( if let Some(build_plan) = build_plan { write_toml_file(&build_plan, build_plan_path).map_err(|inner_err| { let err = Error::CannotWriteBuildPlan(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; } @@ -207,26 +206,28 @@ pub fn libcnb_runtime_build( #[cfg(feature = "trace")] let mut trace = start_trace(&buildpack_descriptor.buildpack, "build"); + let mut record_trace_err = |err: &dyn std::error::Error| { + #[cfg(feature = "trace")] + trace.set_error(err); + }; + let stack_id: StackId = env::var("CNB_STACK_ID") .map_err(Error::CannotDetermineStackId) .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) .map_err(|err| { - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; let platform = Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { let err = Error::CannotCreatePlatformFromPath(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; let buildpack_plan = read_toml_file(&args.buildpack_plan_path).map_err(|inner_err| { let err = Error::CannotReadBuildpackPlan(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; @@ -234,10 +235,9 @@ pub fn libcnb_runtime_build( Err(TomlFileError::IoError(io_error)) if is_not_found_error_kind(&io_error) => Ok(None), other => other.map(Some), } - .map_err(|inner_err| { - let err = Error::CannotReadStore(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + .map_err(Error::CannotReadStore) + .map_err(|err| { + record_trace_err(&err); err })?; @@ -252,12 +252,10 @@ pub fn libcnb_runtime_build( store, }; - let build_result = buildpack.build(build_context); - - #[cfg(feature = "trace")] - if let Err(ref err) = build_result { - trace.set_error(err); - } + let build_result = buildpack.build(build_context).map_err(|err| { + record_trace_err(&err); + err + }); match build_result?.0 { InnerBuildResult::Pass { @@ -269,8 +267,7 @@ pub fn libcnb_runtime_build( if let Some(launch) = launch { write_toml_file(&launch, layers_dir.join("launch.toml")).map_err(|inner_err| { let err = Error::CannotWriteLaunch(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; }; @@ -278,8 +275,7 @@ pub fn libcnb_runtime_build( if let Some(store) = store { write_toml_file(&store, layers_dir.join("store.toml")).map_err(|inner_err| { let err = Error::CannotWriteStore(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + record_trace_err(&err); err })?; }; @@ -289,10 +285,9 @@ pub fn libcnb_runtime_build( cnb_sbom_path(&build_sbom.format, &layers_dir, "build"), &build_sbom.data, ) - .map_err(|inner_err| { - let err = Error::CannotWriteBuildSbom(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + .map_err(Error::CannotWriteBuildSbom) + .map_err(|err| { + record_trace_err(&err); err })?; } @@ -302,10 +297,9 @@ pub fn libcnb_runtime_build( cnb_sbom_path(&launch_sbom.format, &layers_dir, "launch"), &launch_sbom.data, ) - .map_err(|inner_err| { - let err = Error::CannotWriteLaunchSbom(inner_err); - #[cfg(feature = "trace")] - trace.set_error(&err); + .map_err(Error::CannotWriteLaunchSbom) + .map_err(|err| { + record_trace_err(&err); err })?; } From 244fe14b88c314402b4066d85b4f2e8c629dfd4e Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 11:48:27 -0600 Subject: [PATCH 33/37] Better name for closure --- libcnb/src/runtime.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 026108e0..f5563b0f 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -132,7 +132,7 @@ pub fn libcnb_runtime_detect( #[cfg(feature = "trace")] let mut trace = start_trace(&buildpack_descriptor.buildpack, "detect"); - let mut record_trace_err = |err: &dyn std::error::Error| { + let mut trace_error = |err: &dyn std::error::Error| { #[cfg(feature = "trace")] trace.set_error(err); }; @@ -140,13 +140,13 @@ pub fn libcnb_runtime_detect( .map_err(Error::CannotDetermineStackId) .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) .map_err(|err| { - record_trace_err(&err); + trace_error(&err); err })?; let platform = B::Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { let err = Error::CannotCreatePlatformFromPath(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; @@ -161,7 +161,7 @@ pub fn libcnb_runtime_detect( }; let detect_result = buildpack.detect(detect_context).map_err(|err| { - record_trace_err(&err); + trace_error(&err); err }); @@ -175,7 +175,7 @@ pub fn libcnb_runtime_detect( if let Some(build_plan) = build_plan { write_toml_file(&build_plan, build_plan_path).map_err(|inner_err| { let err = Error::CannotWriteBuildPlan(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; } @@ -206,7 +206,7 @@ pub fn libcnb_runtime_build( #[cfg(feature = "trace")] let mut trace = start_trace(&buildpack_descriptor.buildpack, "build"); - let mut record_trace_err = |err: &dyn std::error::Error| { + let mut trace_error = |err: &dyn std::error::Error| { #[cfg(feature = "trace")] trace.set_error(err); }; @@ -215,19 +215,19 @@ pub fn libcnb_runtime_build( .map_err(Error::CannotDetermineStackId) .and_then(|stack_id_string| stack_id_string.parse().map_err(Error::StackIdError)) .map_err(|err| { - record_trace_err(&err); + trace_error(&err); err })?; let platform = Platform::from_path(&args.platform_dir_path).map_err(|inner_err| { let err = Error::CannotCreatePlatformFromPath(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; let buildpack_plan = read_toml_file(&args.buildpack_plan_path).map_err(|inner_err| { let err = Error::CannotReadBuildpackPlan(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; @@ -237,7 +237,7 @@ pub fn libcnb_runtime_build( } .map_err(Error::CannotReadStore) .map_err(|err| { - record_trace_err(&err); + trace_error(&err); err })?; @@ -253,7 +253,7 @@ pub fn libcnb_runtime_build( }; let build_result = buildpack.build(build_context).map_err(|err| { - record_trace_err(&err); + trace_error(&err); err }); @@ -267,7 +267,7 @@ pub fn libcnb_runtime_build( if let Some(launch) = launch { write_toml_file(&launch, layers_dir.join("launch.toml")).map_err(|inner_err| { let err = Error::CannotWriteLaunch(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; }; @@ -275,7 +275,7 @@ pub fn libcnb_runtime_build( if let Some(store) = store { write_toml_file(&store, layers_dir.join("store.toml")).map_err(|inner_err| { let err = Error::CannotWriteStore(inner_err); - record_trace_err(&err); + trace_error(&err); err })?; }; @@ -287,7 +287,7 @@ pub fn libcnb_runtime_build( ) .map_err(Error::CannotWriteBuildSbom) .map_err(|err| { - record_trace_err(&err); + trace_error(&err); err })?; } @@ -299,7 +299,7 @@ pub fn libcnb_runtime_build( ) .map_err(Error::CannotWriteLaunchSbom) .map_err(|err| { - record_trace_err(&err); + trace_error(&err); err })?; } From e11cb55bf7617725e76d5dd2f721e7ecf7f4818f Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 11:50:27 -0600 Subject: [PATCH 34/37] Add -u to bash test buildpacks --- .../tracing/tests/fixtures/buildpacks/tracing-reader/bin/build | 2 +- .../tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build index c2f5004b..e09860e8 100755 --- a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build +++ b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/build @@ -1,5 +1,5 @@ #!/usr/bin/env bash -set -eo pipefail +set -euo pipefail echo "---> Tracing Reader Buildpack" diff --git a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect index 45111a15..6e877466 100755 --- a/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect +++ b/test-buildpacks/tracing/tests/fixtures/buildpacks/tracing-reader/bin/detect @@ -1,2 +1,2 @@ #!/usr/bin/env bash -set -eo pipefail +set -euo pipefail From 74a542092172fa26eccb8f5a6554e45707456850 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 12:24:33 -0600 Subject: [PATCH 35/37] More rigorous assertions for tracing contents --- libcnb/src/tracing.rs | 53 +++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 61c40078..17d0b9c9 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -135,7 +135,7 @@ mod tests { fn test_tracing() { let buildpack = Buildpack { id: buildpack_id!("company.com/foo"), - version: BuildpackVersion::new(0, 0, 0), + version: BuildpackVersion::new(0, 0, 99), name: Some("Foo buildpack for company.com".to_string()), homepage: None, clear_env: false, @@ -144,16 +144,13 @@ mod tests { licenses: vec![], sbom_formats: HashSet::new(), }; - let phase = "bar"; - let event = "baz-event"; - let error_message = "its broken"; let telemetry_path = "/tmp/libcnb-telemetry/company_com_foo-bar.jsonl"; _ = fs::remove_file(telemetry_path); { - let mut trace = start_trace(&buildpack, phase); - trace.add_event(event); - trace.set_error(&Error::new(ErrorKind::Other, error_message)); + let mut trace = start_trace(&buildpack, "bar"); + trace.add_event("baz-event"); + trace.set_error(&Error::new(ErrorKind::Other, "it's broken")); } let tracing_contents = fs::read_to_string(telemetry_path) .expect("Expected telemetry file to exist, but couldn't read it"); @@ -161,13 +158,39 @@ mod tests { println!("tracing_contents: {tracing_contents}"); let _tracing_data: Value = serde_json::from_str(&tracing_contents) .expect("Expected tracing export file contents to be valid json"); - assert!(tracing_contents.contains(phase)); - assert!(tracing_contents.contains(event)); - assert!(tracing_contents.contains(error_message)); - assert!(tracing_contents.contains(buildpack.id.as_str())); - assert!(tracing_contents.contains(&buildpack.version.to_string())); - assert!( - tracing_contents.contains(&buildpack.name.expect("Expected buildpack.name to exist")) - ); + + // Check resource attributes + assert!(tracing_contents.contains( + "{\"key\":\"service.name\",\"value\":{\"stringValue\":\"company.com/foo\"}}" + )); + assert!(tracing_contents + .contains("{\"key\":\"service.version\",\"value\":{\"stringValue\":\"0.0.99\"}}")); + + // Check span name + assert!(tracing_contents.contains("\"name\":\"company_com_foo-bar\"")); + + // Check span attributes + assert!(tracing_contents.contains( + "{\"key\":\"buildpack_id\",\"value\":{\"stringValue\":\"company.com/foo\"}}" + )); + assert!(tracing_contents + .contains("{\"key\":\"buildpack_version\",\"value\":{\"stringValue\":\"0.0.99\"}}")); + assert!(tracing_contents.contains( + "{\"key\":\"buildpack_name\",\"value\":{\"stringValue\":\"Foo buildpack for company.com\"}}" + )); + + // Check event name + assert!(tracing_contents.contains("\"name\":\"baz-event\"")); + + // Check exception event + assert!(tracing_contents.contains("\"name\":\"exception\"")); + assert!(tracing_contents.contains( + "{\"key\":\"exception.message\",\"value\":{\"stringValue\":\"it's broken\"}}" + )); + + // Check error status + assert!(tracing_contents + .contains("\"message\":\"Custom { kind: Other, error: \\\"it's broken\\\" }")); + assert!(tracing_contents.contains("\"code\":1")); } } From afe1d86a59e3180e78098f944e3c2211c648adc3 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 12:27:20 -0600 Subject: [PATCH 36/37] Move ? to a more traditional spot --- libcnb/src/runtime.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index f5563b0f..7b4c85a2 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -163,9 +163,9 @@ pub fn libcnb_runtime_detect( let detect_result = buildpack.detect(detect_context).map_err(|err| { trace_error(&err); err - }); + })?; - match detect_result?.0 { + match detect_result.0 { InnerDetectResult::Fail => { #[cfg(feature = "trace")] trace.add_event("detect-failed"); @@ -255,9 +255,9 @@ pub fn libcnb_runtime_build( let build_result = buildpack.build(build_context).map_err(|err| { trace_error(&err); err - }); + })?; - match build_result?.0 { + match build_result.0 { InnerBuildResult::Pass { launch, store, From e109d7410635e05f0e568e0db9f5d5d10b75c929 Mon Sep 17 00:00:00 2001 From: Josh W Lewis Date: Tue, 5 Dec 2023 13:54:03 -0600 Subject: [PATCH 37/37] Add OpenTelemetry to clippy valid idents rather than allowing doc_markdown issues --- clippy.toml | 1 + libcnb/src/tracing.rs | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy.toml b/clippy.toml index 154626ef..4ee7a54a 100644 --- a/clippy.toml +++ b/clippy.toml @@ -1 +1,2 @@ allow-unwrap-in-tests = true +doc-valid-idents = ["OpenTelemetry", ".."] diff --git a/libcnb/src/tracing.rs b/libcnb/src/tracing.rs index 17d0b9c9..2d0eae80 100644 --- a/libcnb/src/tracing.rs +++ b/libcnb/src/tracing.rs @@ -1,4 +1,3 @@ -#![allow(clippy::doc_markdown)] use libcnb_data::buildpack::Buildpack; use opentelemetry::{ global,