Skip to content

Commit

Permalink
fix: otel resiliency (#26857)
Browse files Browse the repository at this point in the history
Improving the breadth of collected data, and ensuring that the collected
data is more likely to be successfully reported.

- Use `log` crate in more places
- Hook up `log` crate to otel
- Switch to process-wide otel processors
- Handle places that use `process::exit`

Also adds a more robust testing framework, with a deterministic tracing
setting.

Refs: #26852
  • Loading branch information
devsnek authored Nov 14, 2024
1 parent cb107a7 commit 4e899d4
Show file tree
Hide file tree
Showing 37 changed files with 411 additions and 175 deletions.
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ jsonc-parser = { version = "=0.26.2", features = ["serde"] }
lazy-regex = "3"
libc = "0.2.126"
libz-sys = { version = "1.1.20", default-features = false }
log = "0.4.20"
log = { version = "0.4.20", features = ["kv"] }
lsp-types = "=0.97.0" # used by tower-lsp and "proposed" feature is unstable in patch releases
memmem = "0.1.1"
monch = "=0.5.0"
Expand Down
19 changes: 19 additions & 0 deletions cli/args/flags.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ use deno_path_util::normalize_path;
use deno_path_util::url_to_file_path;
use deno_runtime::deno_permissions::PermissionsOptions;
use deno_runtime::deno_permissions::SysDescriptor;
use deno_runtime::ops::otel::OtelConfig;
use log::debug;
use log::Level;
use serde::Deserialize;
Expand Down Expand Up @@ -967,6 +968,24 @@ impl Flags {
args
}

pub fn otel_config(&self) -> Option<OtelConfig> {
if self
.unstable_config
.features
.contains(&String::from("otel"))
{
Some(OtelConfig {
runtime_name: Cow::Borrowed("deno"),
runtime_version: Cow::Borrowed(crate::version::DENO_VERSION_INFO.deno),
deterministic: std::env::var("DENO_UNSTABLE_OTEL_DETERMINISTIC")
.is_ok(),
..Default::default()
})
} else {
None
}
}

/// Extract the paths the config file should be discovered from.
///
/// Returns `None` if the config file should not be auto-discovered.
Expand Down
19 changes: 2 additions & 17 deletions cli/args/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,10 +823,8 @@ impl CliOptions {
};
let msg =
format!("DANGER: TLS certificate validation is disabled {}", domains);
#[allow(clippy::print_stderr)]
{
// use eprintln instead of log::warn so this always gets shown
eprintln!("{}", colors::yellow(msg));
log::error!("{}", colors::yellow(msg));
}
}

Expand Down Expand Up @@ -1131,20 +1129,7 @@ impl CliOptions {
}

pub fn otel_config(&self) -> Option<OtelConfig> {
if self
.flags
.unstable_config
.features
.contains(&String::from("otel"))
{
Some(OtelConfig {
runtime_name: Cow::Borrowed("deno"),
runtime_version: Cow::Borrowed(crate::version::DENO_VERSION_INFO.deno),
..Default::default()
})
} else {
None
}
self.flags.otel_config()
}

pub fn env_file_name(&self) -> Option<&String> {
Expand Down
1 change: 1 addition & 0 deletions cli/clippy.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
disallowed-methods = [
{ path = "reqwest::Client::new", reason = "create an HttpClient via an HttpClientProvider instead" },
{ path = "std::process::exit", reason = "use deno_runtime::exit instead" },
]
disallowed-types = [
{ path = "reqwest::Client", reason = "use crate::http_util::HttpClient instead" },
Expand Down
2 changes: 1 addition & 1 deletion cli/graph_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ pub fn graph_exit_integrity_errors(graph: &ModuleGraph) {
fn exit_for_integrity_error(err: &ModuleError) {
if let Some(err_message) = enhanced_integrity_error_message(err) {
log::error!("{} {}", colors::red("error:"), err_message);
std::process::exit(10);
deno_runtime::exit(10);
}
}

Expand Down
2 changes: 1 addition & 1 deletion cli/lsp/parent_process_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub fn start(parent_process_id: u32) {
std::thread::sleep(Duration::from_secs(10));

if !is_process_active(parent_process_id) {
std::process::exit(1);
deno_runtime::exit(1);
}
});
}
Expand Down
28 changes: 17 additions & 11 deletions cli/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,18 +350,17 @@ fn setup_panic_hook() {
eprintln!("Args: {:?}", env::args().collect::<Vec<_>>());
eprintln!();
orig_hook(panic_info);
std::process::exit(1);
deno_runtime::exit(1);
}));
}

#[allow(clippy::print_stderr)]
fn exit_with_message(message: &str, code: i32) -> ! {
eprintln!(
log::error!(
"{}: {}",
colors::red_bold("error"),
message.trim_start_matches("error: ")
);
std::process::exit(code);
deno_runtime::exit(code);
}

fn exit_for_error(error: AnyError) -> ! {
Expand All @@ -380,13 +379,12 @@ fn exit_for_error(error: AnyError) -> ! {
exit_with_message(&error_string, error_code);
}

#[allow(clippy::print_stderr)]
pub(crate) fn unstable_exit_cb(feature: &str, api_name: &str) {
eprintln!(
log::error!(
"Unstable API '{api_name}'. The `--unstable-{}` flag must be provided.",
feature
);
std::process::exit(70);
deno_runtime::exit(70);
}

pub fn main() {
Expand Down Expand Up @@ -419,7 +417,7 @@ pub fn main() {
drop(profiler);

match result {
Ok(exit_code) => std::process::exit(exit_code),
Ok(exit_code) => deno_runtime::exit(exit_code),
Err(err) => exit_for_error(err),
}
}
Expand All @@ -433,12 +431,21 @@ fn resolve_flags_and_init(
if err.kind() == clap::error::ErrorKind::DisplayVersion =>
{
// Ignore results to avoid BrokenPipe errors.
util::logger::init(None);
let _ = err.print();
std::process::exit(0);
deno_runtime::exit(0);
}
Err(err) => {
util::logger::init(None);
exit_for_error(AnyError::from(err))
}
Err(err) => exit_for_error(AnyError::from(err)),
};

if let Some(otel_config) = flags.otel_config() {
deno_runtime::ops::otel::init(otel_config)?;
}
util::logger::init(flags.log_level);

// TODO(bartlomieju): remove in Deno v2.5 and hard error then.
if flags.unstable_config.legacy_flag_enabled {
log::warn!(
Expand Down Expand Up @@ -467,7 +474,6 @@ fn resolve_flags_and_init(
deno_core::JsRuntime::init_platform(
None, /* import assertions enabled */ false,
);
util::logger::init(flags.log_level);

Ok(flags)
}
20 changes: 12 additions & 8 deletions cli/mainrt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,21 @@ use std::env::current_exe;

use crate::args::Flags;

#[allow(clippy::print_stderr)]
pub(crate) fn unstable_exit_cb(feature: &str, api_name: &str) {
eprintln!(
log::error!(
"Unstable API '{api_name}'. The `--unstable-{}` flag must be provided.",
feature
);
std::process::exit(70);
deno_runtime::exit(70);
}

#[allow(clippy::print_stderr)]
fn exit_with_message(message: &str, code: i32) -> ! {
eprintln!(
log::error!(
"{}: {}",
colors::red_bold("error"),
message.trim_start_matches("error: ")
);
std::process::exit(code);
deno_runtime::exit(code);
}

fn unwrap_or_exit<T>(result: Result<T, AnyError>) -> T {
Expand Down Expand Up @@ -89,13 +87,19 @@ fn main() {
let future = async move {
match standalone {
Ok(Some(data)) => {
if let Some(otel_config) = data.metadata.otel_config.clone() {
deno_runtime::ops::otel::init(otel_config)?;
}
util::logger::init(data.metadata.log_level);
load_env_vars(&data.metadata.env_vars_from_env_file);
let exit_code = standalone::run(data).await?;
std::process::exit(exit_code);
deno_runtime::exit(exit_code);
}
Ok(None) => Ok(()),
Err(err) => Err(err),
Err(err) => {
util::logger::init(None);
Err(err)
}
}
};

Expand Down
2 changes: 1 addition & 1 deletion cli/standalone/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
Some(bytes) => bytes,
None => {
log::info!("Download could not be found, aborting");
std::process::exit(1)
deno_runtime::exit(1);
}
};

Expand Down
2 changes: 1 addition & 1 deletion cli/tools/lint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ pub async fn lint(
linter.finish()
};
if !success {
std::process::exit(1);
deno_runtime::exit(1);
}
}

Expand Down
2 changes: 2 additions & 0 deletions cli/tools/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1357,6 +1357,7 @@ pub async fn report_tests(
if let Err(err) = reporter.flush_report(&elapsed, &tests, &test_steps) {
eprint!("Test reporter failed to flush: {}", err)
}
#[allow(clippy::disallowed_methods)]
std::process::exit(130);
}
}
Expand Down Expand Up @@ -1642,6 +1643,7 @@ pub async fn run_tests_with_watch(
loop {
signal::ctrl_c().await.unwrap();
if !HAS_TEST_RUN_SIGINT_HANDLER.load(Ordering::Relaxed) {
#[allow(clippy::disallowed_methods)]
std::process::exit(130);
}
}
Expand Down
2 changes: 1 addition & 1 deletion cli/tools/upgrade.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ pub async fn upgrade(
let Some(archive_data) = download_package(&client, download_url).await?
else {
log::error!("Download could not be found, aborting");
std::process::exit(1)
deno_runtime::exit(1)
};

log::info!(
Expand Down
3 changes: 1 addition & 2 deletions cli/util/file_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ impl DebouncedReceiver {
}
}

#[allow(clippy::print_stderr)]
async fn error_handler<F>(watch_future: F) -> bool
where
F: Future<Output = Result<(), AnyError>>,
Expand All @@ -84,7 +83,7 @@ where
Some(e) => format_js_error(e),
None => format!("{err:?}"),
};
eprintln!(
log::error!(
"{}: {}",
colors::red_bold("error"),
error_string.trim_start_matches("error: ")
Expand Down
1 change: 1 addition & 0 deletions cli/util/logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ impl log::Log for CliLogger {
// thread's state
DrawThread::hide();
self.0.log(record);
deno_runtime::ops::otel::handle_log(record);
DrawThread::show();
}
}
Expand Down
9 changes: 4 additions & 5 deletions cli/util/v8.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,14 @@ pub fn init_v8_flags(
.skip(1)
.collect::<Vec<_>>();

#[allow(clippy::print_stderr)]
if !unrecognized_v8_flags.is_empty() {
for f in unrecognized_v8_flags {
eprintln!("error: V8 did not recognize flag '{f}'");
log::error!("error: V8 did not recognize flag '{f}'");
}
eprintln!("\nFor a list of V8 flags, use '--v8-flags=--help'");
std::process::exit(1);
log::error!("\nFor a list of V8 flags, use '--v8-flags=--help'");
deno_runtime::exit(1);
}
if v8_flags_includes_help {
std::process::exit(0);
deno_runtime::exit(0);
}
}
5 changes: 2 additions & 3 deletions ext/napi/node_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ fn napi_fatal_exception(env: &mut Env, err: napi_value) -> napi_status {
}

#[napi_sym]
#[allow(clippy::print_stderr)]
fn napi_fatal_error(
location: *const c_char,
location_len: usize,
Expand Down Expand Up @@ -173,9 +172,9 @@ fn napi_fatal_error(
};

if let Some(location) = location {
eprintln!("NODE API FATAL ERROR: {} {}", location, message);
log::error!("NODE API FATAL ERROR: {} {}", location, message);
} else {
eprintln!("NODE API FATAL ERROR: {}", message);
log::error!("NODE API FATAL ERROR: {}", message);
}

std::process::abort();
Expand Down
3 changes: 1 addition & 2 deletions ext/napi/uv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@ use deno_core::parking_lot::Mutex;
use std::mem::MaybeUninit;
use std::ptr::addr_of_mut;

#[allow(clippy::print_stderr)]
fn assert_ok(res: c_int) -> c_int {
if res != 0 {
eprintln!("bad result in uv polyfill: {res}");
log::error!("bad result in uv polyfill: {res}");
// don't panic because that might unwind into
// c/c++
std::process::abort();
Expand Down
1 change: 1 addition & 0 deletions runtime/clippy.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,5 @@ disallowed-methods = [
{ path = "std::fs::write", reason = "File system operations should be done using FileSystem trait" },
{ path = "std::path::Path::canonicalize", reason = "File system operations should be done using FileSystem trait" },
{ path = "std::path::Path::exists", reason = "File system operations should be done using FileSystem trait" },
{ path = "std::process::exit", reason = "use deno_runtime::exit instead" },
]
Loading

0 comments on commit 4e899d4

Please sign in to comment.