From cf0ce684247685fbcb499c2fd26142c704abcb78 Mon Sep 17 00:00:00 2001 From: Colin Casey Date: Mon, 28 Oct 2024 15:24:56 -0300 Subject: [PATCH] Remove experimental tracing Removes the otel tracing added in [#652](https://github.com/heroku/buildpacks-nodejs/pull/652) which was introduced before the `libcnb` tracing support in [heroku/libcnb.rs#723](https://github.com/heroku/libcnb.rs/pull/723). This is also blocking the dependency updates in [#942](https://github.com/heroku/buildpacks-nodejs/pull/942). --- Cargo.lock | 66 ++---------------- buildpacks/nodejs-corepack/Cargo.toml | 1 - buildpacks/nodejs-corepack/src/main.rs | 93 ++++++++++---------------- common/nodejs-utils/Cargo.toml | 3 - common/nodejs-utils/src/lib.rs | 1 - common/nodejs-utils/src/telemetry.rs | 82 ----------------------- 6 files changed, 43 insertions(+), 203 deletions(-) delete mode 100644 common/nodejs-utils/src/telemetry.rs diff --git a/Cargo.lock b/Cargo.lock index d899da10..c3e58514 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -622,7 +622,6 @@ dependencies = [ "libcnb", "libcnb-test", "libherokubuildpack 0.23.0", - "opentelemetry 0.24.0", "serde", "test_support", "thiserror", @@ -695,9 +694,6 @@ dependencies = [ "keep_a_changelog_file", "libcnb-data", "node-semver", - "opentelemetry 0.24.0", - "opentelemetry-stdout 0.5.0", - "opentelemetry_sdk 0.24.1", "regex", "serde", "serde-xml-rs", @@ -908,9 +904,9 @@ dependencies = [ "libcnb-common", "libcnb-data", "libcnb-proc-macros", - "opentelemetry 0.21.0", - "opentelemetry-stdout 0.2.0", - "opentelemetry_sdk 0.21.2", + "opentelemetry", + "opentelemetry-stdout", + "opentelemetry_sdk", "serde", "thiserror", "toml", @@ -1152,20 +1148,6 @@ dependencies = [ "urlencoding", ] -[[package]] -name = "opentelemetry" -version = "0.24.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4c365a63eec4f55b7efeceb724f1336f26a9cf3427b70e59e2cd2a5b947fba96" -dependencies = [ - "futures-core", - "futures-sink", - "js-sys", - "once_cell", - "pin-project-lite", - "thiserror", -] - [[package]] name = "opentelemetry-stdout" version = "0.2.0" @@ -1174,28 +1156,11 @@ checksum = "c13b2df4cd59c176099ac82806725ba340c8fa7b1a7004c0912daad30470f63e" dependencies = [ "chrono", "futures-util", - "opentelemetry 0.21.0", - "opentelemetry_sdk 0.21.2", - "ordered-float", - "serde", - "serde_json", -] - -[[package]] -name = "opentelemetry-stdout" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d408d4345b8be6129a77c46c3bfc75f0d3476f3091909c7dd99c1f3d78582287" -dependencies = [ - "async-trait", - "chrono", - "futures-util", - "opentelemetry 0.24.0", - "opentelemetry_sdk 0.24.1", + "opentelemetry", + "opentelemetry_sdk", "ordered-float", "serde", "serde_json", - "thiserror", ] [[package]] @@ -1210,32 +1175,13 @@ dependencies = [ "futures-executor", "futures-util", "once_cell", - "opentelemetry 0.21.0", + "opentelemetry", "ordered-float", "percent-encoding", "rand", "thiserror", ] -[[package]] -name = "opentelemetry_sdk" -version = "0.24.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "692eac490ec80f24a17828d49b40b60f5aeaccdfe6a503f939713afd22bc28df" -dependencies = [ - "async-trait", - "futures-channel", - "futures-executor", - "futures-util", - "glob", - "once_cell", - "opentelemetry 0.24.0", - "percent-encoding", - "rand", - "serde_json", - "thiserror", -] - [[package]] name = "ordered-float" version = "4.2.2" diff --git a/buildpacks/nodejs-corepack/Cargo.toml b/buildpacks/nodejs-corepack/Cargo.toml index 48db46d8..26ac7217 100644 --- a/buildpacks/nodejs-corepack/Cargo.toml +++ b/buildpacks/nodejs-corepack/Cargo.toml @@ -11,7 +11,6 @@ heroku-nodejs-utils.workspace = true indoc = "2" libcnb = { version = "=0.23.0", features = ["trace"] } libherokubuildpack = { version = "=0.23.0", default-features = false, features = ["log"] } -opentelemetry = "0.24" serde = "1" thiserror = "1" diff --git a/buildpacks/nodejs-corepack/src/main.rs b/buildpacks/nodejs-corepack/src/main.rs index fdbcfe97..f00ba15c 100644 --- a/buildpacks/nodejs-corepack/src/main.rs +++ b/buildpacks/nodejs-corepack/src/main.rs @@ -1,5 +1,4 @@ use heroku_nodejs_utils::package_json::{PackageJson, PackageJsonError}; -use heroku_nodejs_utils::telemetry::init_tracer; use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder}; use libcnb::data::build_plan::BuildPlanBuilder; use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder}; @@ -7,8 +6,6 @@ use libcnb::generic::GenericMetadata; use libcnb::generic::GenericPlatform; use libcnb::{buildpack_main, Buildpack, Env}; use libherokubuildpack::log::log_header; -use opentelemetry::trace::{TraceContextExt, Tracer}; -use opentelemetry::KeyValue; use crate::enable_corepack::enable_corepack; use crate::prepare_corepack::prepare_corepack; @@ -35,67 +32,51 @@ impl Buildpack for CorepackBuildpack { type Error = CorepackBuildpackError; fn detect(&self, context: DetectContext) -> libcnb::Result { - let tracer = init_tracer(context.buildpack_descriptor.buildpack.id.to_string()); - tracer.in_span("nodejs-corepack-detect", |_cx| { - // Corepack requires the `packageManager` key from `package.json`. - // This buildpack won't be detected without it. - let pkg_json_path = context.app_dir.join("package.json"); - if pkg_json_path.exists() { - let pkg_json = PackageJson::read(pkg_json_path) - .map_err(CorepackBuildpackError::PackageJson)?; - cfg::get_supported_package_manager(&pkg_json).map_or_else( - || DetectResultBuilder::fail().build(), - |pkg_mgr| { - DetectResultBuilder::pass() - .build_plan( - BuildPlanBuilder::new() - .requires("node") - .requires(&pkg_mgr) - .provides(pkg_mgr) - .build(), - ) - .build() - }, - ) - } else { - DetectResultBuilder::fail().build() - } - }) + // Corepack requires the `packageManager` key from `package.json`. + // This buildpack won't be detected without it. + let pkg_json_path = context.app_dir.join("package.json"); + if pkg_json_path.exists() { + let pkg_json = + PackageJson::read(pkg_json_path).map_err(CorepackBuildpackError::PackageJson)?; + cfg::get_supported_package_manager(&pkg_json).map_or_else( + || DetectResultBuilder::fail().build(), + |pkg_mgr| { + DetectResultBuilder::pass() + .build_plan( + BuildPlanBuilder::new() + .requires("node") + .requires(&pkg_mgr) + .provides(pkg_mgr) + .build(), + ) + .build() + }, + ) + } else { + DetectResultBuilder::fail().build() + } } fn build(&self, context: BuildContext) -> libcnb::Result { - let tracer = init_tracer(context.buildpack_descriptor.buildpack.id.to_string()); - tracer.in_span("nodejs-corepack-build", |cx| { - let pkg_mgr = PackageJson::read(context.app_dir.join("package.json")) - .map_err(CorepackBuildpackError::PackageJson)? - .package_manager - .ok_or(CorepackBuildpackError::PackageManagerMissing)?; + let pkg_mgr = PackageJson::read(context.app_dir.join("package.json")) + .map_err(CorepackBuildpackError::PackageJson)? + .package_manager + .ok_or(CorepackBuildpackError::PackageManagerMissing)?; - cx.span().set_attributes([ - KeyValue::new("package_manager.name", pkg_mgr.name.clone()), - KeyValue::new("package_manager.version", pkg_mgr.version.to_string()), - ]); + let env = &Env::from_current(); - let env = &Env::from_current(); + let corepack_version = + cmd::corepack_version(env).map_err(CorepackBuildpackError::CorepackVersion)?; - let corepack_version = - cmd::corepack_version(env).map_err(CorepackBuildpackError::CorepackVersion)?; + log_header(format!( + "Installing {} {} via corepack {corepack_version}", + pkg_mgr.name, pkg_mgr.version + )); - cx.span().set_attribute(KeyValue::new( - "corepack.version", - corepack_version.to_string(), - )); + enable_corepack(&context, &corepack_version, &pkg_mgr, env)?; + prepare_corepack(&context, &pkg_mgr, env)?; - log_header(format!( - "Installing {} {} via corepack {corepack_version}", - pkg_mgr.name, pkg_mgr.version - )); - - enable_corepack(&context, &corepack_version, &pkg_mgr, env)?; - prepare_corepack(&context, &pkg_mgr, env)?; - - BuildResultBuilder::new().build() - }) + BuildResultBuilder::new().build() } fn on_error(&self, err: libcnb::Error) { diff --git a/common/nodejs-utils/Cargo.toml b/common/nodejs-utils/Cargo.toml index e1478f50..130fe809 100644 --- a/common/nodejs-utils/Cargo.toml +++ b/common/nodejs-utils/Cargo.toml @@ -15,9 +15,6 @@ indoc = "2" keep_a_changelog_file = "0.1.0" libcnb-data = "=0.23.0" node-semver = "2" -opentelemetry = "0.24" -opentelemetry_sdk = { version = "0.24", features = ["trace"] } -opentelemetry-stdout = { version = "0.5", features = ["trace"] } regex = "1" serde = { version = "1", features = ['derive'] } serde_json = "1" diff --git a/common/nodejs-utils/src/lib.rs b/common/nodejs-utils/src/lib.rs index edad3cd9..3e3233da 100644 --- a/common/nodejs-utils/src/lib.rs +++ b/common/nodejs-utils/src/lib.rs @@ -10,5 +10,4 @@ mod npmjs_org; pub mod package_json; pub mod package_manager; mod s3; -pub mod telemetry; pub mod vrs; diff --git a/common/nodejs-utils/src/telemetry.rs b/common/nodejs-utils/src/telemetry.rs deleted file mode 100644 index e8bb0bf5..00000000 --- a/common/nodejs-utils/src/telemetry.rs +++ /dev/null @@ -1,82 +0,0 @@ -use std::{ - fs::{create_dir_all, File}, - path::Path, -}; - -use opentelemetry::{ - global::{self, BoxedTracer}, - KeyValue, -}; -use opentelemetry_sdk::{ - trace::{Config, TracerProvider}, - Resource, -}; - -pub fn init_tracer(buildpack_name: impl Into) -> BoxedTracer { - let bp_name = buildpack_name.into(); - let telem_file_path = Path::new("/tmp") - .join("cnb-telemetry") - .join(format!("{}.jsonl", bp_name.replace('/', "_"))); - if let Some(parent_dir) = telem_file_path.parent() { - let _ = create_dir_all(parent_dir); - } - let exporter = match File::create(telem_file_path) { - Ok(f) => opentelemetry_stdout::SpanExporter::builder() - .with_writer(f) - .build(), - Err(_) => opentelemetry_stdout::SpanExporter::default(), - }; - let provider = TracerProvider::builder() - .with_config( - Config::default().with_resource(Resource::new(vec![KeyValue::new( - "service.name", - bp_name.clone(), - )])), - ) - .with_simple_exporter(exporter) - .build(); - global::set_tracer_provider(provider); - global::tracer(bp_name) -} - -#[cfg(test)] -mod tests { - use std::fs::{self}; - - use super::init_tracer; - use opentelemetry::{global, trace::TraceContextExt, trace::Tracer}; - - #[test] - fn test_tracer_writes_span_file() -> Result<(), String> { - let buildpack_name = "heroku_test_buildpack"; - let test_span_name = "test_span_1"; - let test_event_name = "test_event_1"; - let telemetry_file_path = format!("/tmp/cnb-telemetry/{buildpack_name}.jsonl"); - - let _ = fs::remove_file(&telemetry_file_path); - - init_tracer(buildpack_name.to_string()); - let tracer = global::tracer(""); - tracer.in_span(test_span_name, |cx| { - cx.span().add_event(test_event_name, Vec::new()); - }); - global::shutdown_tracer_provider(); - let contents = fs::read_to_string(telemetry_file_path) - .expect("expected to read existing telemetry file"); - println!("{contents}"); - - if !contents.contains(buildpack_name) { - Err("File export did not include buildpack name")?; - } - - if !contents.contains(test_span_name) { - Err("File export did not include test span")?; - } - - if !contents.contains(test_event_name) { - Err("File export did not include test event")?; - } - - Ok(()) - } -}