From 45251d9ff08fa8a9084140976d9e61b0503f4bd4 Mon Sep 17 00:00:00 2001 From: Severin Siffert Date: Thu, 23 Jan 2025 18:50:27 +0100 Subject: [PATCH] feat!: reduce error output verbosity (#4067) --- CHANGELOG.md | 2 + e2e/tests-dfx/basic-project.bash | 3 +- e2e/tests-dfx/call.bash | 4 +- src/dfx/src/commands/canister/call.rs | 7 +- .../src/commands/canister/update_settings.rs | 4 +- src/dfx/src/lib/diagnosis.rs | 83 +++++++++---------- .../operations/canister/motoko_playground.rs | 8 +- src/dfx/src/lib/operations/ledger.rs | 2 +- src/dfx/src/main.rs | 64 +++++++------- 9 files changed, 90 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72ced8596c..50963de5d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -160,6 +160,8 @@ Total query response payload size: 0 Bytes Log visibility: controllers ``` +### feat!: Print error traces only in verbose (`-v`) mode or if no proper error message is available + ## Dependencies ### Frontend canister diff --git a/e2e/tests-dfx/basic-project.bash b/e2e/tests-dfx/basic-project.bash index 4ef61a7ce9..1131673b79 100644 --- a/e2e/tests-dfx/basic-project.bash +++ b/e2e/tests-dfx/basic-project.bash @@ -79,7 +79,8 @@ teardown() { assert_eq "4449444c00017d02" assert_command_fail dfx canister call --query hello_backend inc - assert_match "Not a query method." + assert_match "inc is an update method, not a query method." + assert_match "Run the command without '--query'." dfx canister call hello_backend inc diff --git a/e2e/tests-dfx/call.bash b/e2e/tests-dfx/call.bash index e1c18fc82c..53cba42ff7 100644 --- a/e2e/tests-dfx/call.bash +++ b/e2e/tests-dfx/call.bash @@ -311,7 +311,7 @@ function impersonate_sender() { # updating settings now fails because the default identity does not control the canister anymore assert_command_fail dfx canister update-settings hello_backend --freezing-threshold 0 - assert_contains "Only controllers of canister $CANISTER_ID can call ic00 method update_settings" + assert_contains "The principal you are using to call a management function is not part of the controllers." # updating settings succeeds when impersonating the management canister as the sender assert_command dfx canister update-settings hello_backend --freezing-threshold 0 --impersonate "${IDENTITY_PRINCIPAL}" @@ -322,7 +322,7 @@ function impersonate_sender() { # canister status fails because the default identity does not control the canister anymore assert_command_fail dfx canister status hello_backend - assert_contains "Only controllers of canister $CANISTER_ID can call ic00 method canister_status" + assert_contains "The principal you are using to call a management function is not part of the controllers." # canister status succeeds when impersonating the management canister as the sender assert_command dfx canister status hello_backend --impersonate "${IDENTITY_PRINCIPAL}" diff --git a/src/dfx/src/commands/canister/call.rs b/src/dfx/src/commands/canister/call.rs index abb382d764..1841020aee 100644 --- a/src/dfx/src/commands/canister/call.rs +++ b/src/dfx/src/commands/canister/call.rs @@ -337,8 +337,11 @@ To figure out the id of your wallet, run 'dfx identity get-wallet (--network ic) let cycles = opts.with_cycles.unwrap_or(0); if call_sender == &CallSender::SelectedId && cycles != 0 { - return Err(DiagnosedError::new("It is only possible to send cycles from a canister.".to_string(), "To send the same function call from your wallet (a canister), run the command using 'dfx canister call (--network ic) --wallet '.\n\ - To figure out the id of your wallet, run 'dfx identity get-wallet (--network ic)'.".to_string())).context("Function caller is not a canister."); + let explanation = "It is only possible to send cycles from a canister."; + let action_suggestion = "To send the same function call from your wallet (a canister), run the command using 'dfx canister call (--network ic) --wallet '.\n\ + To figure out the id of your wallet, run 'dfx identity get-wallet (--network ic)'."; + return Err(DiagnosedError::new(explanation, action_suggestion)) + .context("Function caller is not a canister."); } if is_query { diff --git a/src/dfx/src/commands/canister/update_settings.rs b/src/dfx/src/commands/canister/update_settings.rs index d1ae33fb1a..d55c6fd6a1 100644 --- a/src/dfx/src/commands/canister/update_settings.rs +++ b/src/dfx/src/commands/canister/update_settings.rs @@ -137,8 +137,8 @@ pub async fn exec( if threshold_in_seconds > 50_000_000 /* ~1.5 years */ && !opts.confirm_very_long_freezing_threshold { return Err(DiagnosedError::new( - "The freezing threshold is defined in SECONDS before the canister would run out of cycles, not in cycles.".to_string(), - "If you truly want to set a freezing threshold that is longer than a year, please run the same command, but with the flag --confirm-very-long-freezing-threshold to confirm you want to do this.".to_string(), + "The freezing threshold is defined in SECONDS before the canister would run out of cycles, not in cycles.", + "If you truly want to set a freezing threshold that is longer than a year, please run the same command, but with the flag --confirm-very-long-freezing-threshold to confirm you want to do this.", )).context("Misunderstanding is very likely."); } } diff --git a/src/dfx/src/lib/diagnosis.rs b/src/dfx/src/lib/diagnosis.rs index 49bd3b2b77..ee122da3f1 100644 --- a/src/dfx/src/lib/diagnosis.rs +++ b/src/dfx/src/lib/diagnosis.rs @@ -10,18 +10,12 @@ use regex::Regex; use std::path::Path; use thiserror::Error as ThisError; -/// Contains two Option that can be displayed to the user: -/// - Error explanation: Goes into a bit of detail on what the error is and/or where the user can find out more about it. -/// - Action suggestion: Tells the user how to move forward to resolve the error. -pub type Diagnosis = (Option, Option); -pub const NULL_DIAGNOSIS: Diagnosis = (None, None); - -#[derive(ThisError, Debug)] +#[derive(ThisError, Debug, Default, Clone)] /// If you do not need the generic error diagnosis to run, you can add a DiagnosedError with .context(err: DiagnosedError). /// In that case, no extra diagnosis is attempted and the last-added explanation and suggestion are printed out. pub struct DiagnosedError { /// A user-friendly explanation of what went wrong. - pub error_explanation: Option, + pub explanation: Option, /// Suggestions for the user on how to move forward to recover from the error. pub action_suggestion: Option, @@ -34,21 +28,22 @@ impl std::fmt::Display for DiagnosedError { } impl DiagnosedError { - pub fn new(error_explanation: String, action_suggestion: String) -> Self { + pub fn new(explanation: impl Into, action_suggestion: impl Into) -> Self { Self { - error_explanation: Some(error_explanation), - action_suggestion: Some(action_suggestion), + explanation: Some(explanation.into()), + action_suggestion: Some(action_suggestion.into()), } } + + pub fn contains_diagnosis(&self) -> bool { + self.action_suggestion.is_some() || self.explanation.is_some() + } } /// Attempts to give helpful suggestions on how to resolve errors. -pub fn diagnose(err: &AnyhowError) -> Diagnosis { +pub fn diagnose(err: &AnyhowError) -> DiagnosedError { if let Some(diagnosed_error) = err.downcast_ref::() { - return ( - diagnosed_error.error_explanation.clone(), - diagnosed_error.action_suggestion.clone(), - ); + return diagnosed_error.clone(); } if let Some(agent_err) = err.downcast_ref::() { @@ -84,7 +79,7 @@ pub fn diagnose(err: &AnyhowError) -> Diagnosis { } } - NULL_DIAGNOSIS + DiagnosedError::default() } fn local_replica_not_running(err: &AnyhowError) -> bool { @@ -127,11 +122,12 @@ fn not_a_controller(err: &AgentError) -> bool { } // Older replicas do not include the error code in the reject response. - // differing behavior between replica and ic-ref: - // replica gives HTTP403, ic-ref gives HTTP400 with message "Wrong sender" - matches!(err, AgentError::HttpError(payload) if payload.status == 403) - || matches!(err, AgentError::HttpError(payload) if payload.status == 400 && - matches!(std::str::from_utf8(payload.content.as_slice()), Ok("Wrong sender"))) + // replica gives HTTP403, message looks like "Only controllers of canister bkyz2-fmaaa-aaaaa-qaaaq-cai can call ic00 method canister_status" + matches!(err, AgentError::HttpError(payload) if payload.status == 403 + && matches!( + std::str::from_utf8(payload.content.as_slice()), + Ok("Only controllers of canister") + )) } fn wallet_method_not_found(err: &AgentError) -> bool { @@ -150,8 +146,8 @@ fn wallet_method_not_found(err: &AgentError) -> bool { } } -fn diagnose_http_403() -> Diagnosis { - let error_explanation = "Each canister has a set of controllers. Only those controllers have access to the canister's management functions (like install_code or stop_canister).\n\ +fn diagnose_http_403() -> DiagnosedError { + let explanation = "Each canister has a set of controllers. Only those controllers have access to the canister's management functions (like install_code or stop_canister).\n\ The principal you are using to call a management function is not part of the controllers."; let action_suggestion = "To make the management function call succeed, you have to make sure the principal that calls the function is a controller. To see the current controllers of a canister, use the 'dfx canister info (--network ic)' command. @@ -162,26 +158,21 @@ To add a principal to the list of controllers, one of the existing controllers h If your wallet is a controller, but not your own principal, then you have to make your wallet perform the call by adding '--wallet ' to the command. The most common way this error is solved is by running 'dfx canister update-settings --network ic --wallet \"$(dfx identity get-wallet)\" --all --add-controller \"$(dfx identity get-principal)\"'."; - ( - Some(error_explanation.to_string()), - Some(action_suggestion.to_string()), - ) + DiagnosedError::new(explanation, action_suggestion) } -fn diagnose_local_replica_not_running() -> Diagnosis { - let error_explanation = +fn diagnose_local_replica_not_running() -> DiagnosedError { + let explanation = "You are trying to connect to the local replica but dfx cannot connect to it."; let action_suggestion = "Target a different network or run 'dfx start' to start the local replica."; - ( - Some(error_explanation.to_string()), - Some(action_suggestion.to_string()), - ) + DiagnosedError::new(explanation, action_suggestion) } -fn subnet_not_authorized() -> Diagnosis { +fn subnet_not_authorized() -> DiagnosedError { + let explanation = "Subnet is not authorized to respond for the requested canister id."; let action_suggestion = "If you are connecting to a node directly instead of a boundary node, try using --provisional-create-canister-effective-canister-id with a canister id in the subnet's canister range. First non-root subnet: 5v3p4-iyaaa-aaaaa-qaaaa-cai, second non-root subnet: jrlun-jiaaa-aaaab-aaaaa-cai"; - (None, Some(action_suggestion.to_string())) + DiagnosedError::new(explanation, action_suggestion) } fn duplicate_asset_key_dist_and_src(sync_error: &SyncError) -> bool { @@ -208,7 +199,7 @@ fn duplicate_asset_key_dist_and_src(sync_error: &SyncError) -> bool { ) } -fn diagnose_duplicate_asset_key_dist_and_src() -> Diagnosis { +fn diagnose_duplicate_asset_key_dist_and_src() -> DiagnosedError { let explanation = "An asset key was found in both the dist and src directories. One or both of the following are a likely explanation: - webpack.config.js is configured to copy assets from the src directory to the dist/ directory. @@ -227,10 +218,10 @@ One or both of the following are a likely explanation: See also release notes: https://forum.dfinity.org/t/dfx-0-11-0-is-promoted-with-breaking-changes/14327"#; - (Some(explanation.to_string()), Some(suggestion.to_string())) + DiagnosedError::new(explanation, suggestion) } -fn diagnose_bad_wallet() -> Diagnosis { +fn diagnose_bad_wallet() -> DiagnosedError { let explanation = "\ A wallet has been previously configured (e.g. via `dfx identity set-wallet`). However, it did not contain a function that dfx was looking for. @@ -245,7 +236,7 @@ wrong, you can set a new wallet with `dfx identity set-wallet --identity `. If you're using a local replica and configuring a wallet was a mistake, you can recreate the replica with `dfx stop && dfx start --clean` to start over."; - (Some(explanation.to_string()), Some(suggestion.to_string())) + DiagnosedError::new(explanation, suggestion) } fn cycles_ledger_not_found(err: &AnyhowError) -> bool { @@ -253,13 +244,13 @@ fn cycles_ledger_not_found(err: &AnyhowError) -> bool { .contains("Canister um5iw-rqaaa-aaaaq-qaaba-cai not found") } -fn diagnose_cycles_ledger_not_found() -> Diagnosis { +fn diagnose_cycles_ledger_not_found() -> DiagnosedError { let explanation = "Cycles ledger with canister ID 'um5iw-rqaaa-aaaaq-qaaba-cai' is not installed."; let suggestion = "Run the command with '--ic' flag if you want to manage the cycles on the mainnet."; - (Some(explanation.to_string()), Some(suggestion.to_string())) + DiagnosedError::new(explanation, suggestion) } fn ledger_not_found(err: &AnyhowError) -> bool { @@ -267,19 +258,19 @@ fn ledger_not_found(err: &AnyhowError) -> bool { .contains("Canister ryjl3-tyaaa-aaaaa-aaaba-cai not found") } -fn diagnose_ledger_not_found() -> Diagnosis { +fn diagnose_ledger_not_found() -> DiagnosedError { let explanation = "ICP Ledger with canister ID 'ryjl3-tyaaa-aaaaa-aaaba-cai' is not installed."; let suggestion = "Run the command with '--ic' flag if you want to manage the ICP on the mainnet."; - (Some(explanation.to_string()), Some(suggestion.to_string())) + DiagnosedError::new(explanation, suggestion) } fn insufficient_cycles(err: &CreateCanisterError) -> bool { matches!(err, CreateCanisterError::InsufficientFunds { balance: _ }) } -fn diagnose_insufficient_cycles() -> Diagnosis { +fn diagnose_insufficient_cycles() -> DiagnosedError { let network = match get_network_context() { Ok(value) => { if value == "local" { @@ -297,5 +288,5 @@ fn diagnose_insufficient_cycles() -> Diagnosis { 'dfx cycles convert --amount=0.123{}'", network ); - (Some(explanation.to_string()), Some(suggestion)) + DiagnosedError::new(explanation, suggestion) } diff --git a/src/dfx/src/lib/operations/canister/motoko_playground.rs b/src/dfx/src/lib/operations/canister/motoko_playground.rs index 6bc9618933..71a84bf84f 100644 --- a/src/dfx/src/lib/operations/canister/motoko_playground.rs +++ b/src/dfx/src/lib/operations/canister/motoko_playground.rs @@ -228,10 +228,10 @@ pub async fn playground_install_code( .await .map_err(|err| { if is_asset_canister && err.to_string().contains("Wasm is not whitelisted") { - anyhow!(DiagnosedError { - error_explanation: Some("The frontend canister wasm needs to be allowlisted in the playground but it isn't. This is a mistake in the release process.".to_string()), - action_suggestion: Some("Please report this on forum.dfinity.org and mention your dfx version. You can get the version with 'dfx --version'.".to_string()), - }) + anyhow!(DiagnosedError::new( + "The frontend canister wasm needs to be allowlisted in the playground but it isn't. This is a mistake in the release process.", + "Please report this on forum.dfinity.org and mention your dfx version. You can get the version with 'dfx --version'." + )) } else { anyhow!(err) } diff --git a/src/dfx/src/lib/operations/ledger.rs b/src/dfx/src/lib/operations/ledger.rs index 37f882ea79..66f7eac2a9 100644 --- a/src/dfx/src/lib/operations/ledger.rs +++ b/src/dfx/src/lib/operations/ledger.rs @@ -191,7 +191,7 @@ Your principal for ICP wallets and decentralized exchanges: {} principal.to_text() ); - DiagnosedError::new(explanation.to_string(), suggestion) + DiagnosedError::new(explanation, suggestion) } fn retryable(agent_error: &AgentError) -> bool { diff --git a/src/dfx/src/main.rs b/src/dfx/src/main.rs index 85603b4657..fbdbdeece0 100644 --- a/src/dfx/src/main.rs +++ b/src/dfx/src/main.rs @@ -1,6 +1,6 @@ #![allow(special_module_name)] use crate::config::{dfx_version, dfx_version_str}; -use crate::lib::diagnosis::{diagnose, Diagnosis}; +use crate::lib::diagnosis::{diagnose, DiagnosedError}; use crate::lib::environment::{Environment, EnvironmentImpl}; use crate::lib::error::DfxResult; use crate::lib::logger::{create_root_logger, LoggingMode}; @@ -67,48 +67,52 @@ fn setup_logging(opts: &CliOpts) -> (i64, slog::Logger) { (verbose_level, create_root_logger(verbose_level, mode)) } -fn print_error_and_diagnosis(err: Error, error_diagnosis: Diagnosis) { +fn print_error_and_diagnosis(log_level: Option, err: Error, error_diagnosis: DiagnosedError) { let mut stderr = util::stderr_wrapper::stderr_wrapper(); // print error chain stack - for (level, cause) in err.chain().enumerate() { - if cause.to_string().is_empty() { - continue; + if log_level.unwrap_or_default() > 0 // DEBUG or more verbose + || !error_diagnosis.contains_diagnosis() + { + for (level, cause) in err.chain().enumerate() { + if cause.to_string().is_empty() { + continue; + } + + let (color, prefix) = if level == 0 { + (term::color::RED, "Error") + } else { + (term::color::YELLOW, "Caused by") + }; + stderr + .fg(color) + .expect("Failed to set stderr output color."); + write!(stderr, "{prefix}: ").expect("Failed to write to stderr."); + stderr + .reset() + .expect("Failed to reset stderr output color."); + + writeln!(stderr, "{cause}").expect("Failed to write to stderr."); } - - let (color, prefix) = if level == 0 { - (term::color::RED, "Error") - } else { - (term::color::YELLOW, "Caused by") - }; - stderr - .fg(color) - .expect("Failed to set stderr output color."); - write!(stderr, "{prefix}: ").expect("Failed to write to stderr."); - stderr - .reset() - .expect("Failed to reset stderr output color."); - - writeln!(stderr, "{cause}").expect("Failed to write to stderr."); } // print diagnosis - if let Some(error_explanation) = error_diagnosis.0 { + if let Some(explanation) = error_diagnosis.explanation { stderr - .fg(term::color::YELLOW) + .fg(term::color::RED) .expect("Failed to set stderr output color."); - writeln!(stderr, "Error explanation:").expect("Failed to write to stderr."); + write!(stderr, "Error: ").expect("Failed to write to stderr."); stderr .reset() .expect("Failed to reset stderr output color."); - writeln!(stderr, "{}", error_explanation).expect("Failed to write to stderr."); + writeln!(stderr, "{}", explanation).expect("Failed to write to stderr."); } - if let Some(action_suggestion) = error_diagnosis.1 { + if let Some(action_suggestion) = error_diagnosis.action_suggestion { stderr .fg(term::color::YELLOW) .expect("Failed to set stderr output color."); - writeln!(stderr, "How to resolve the error:").expect("Failed to write to stderr."); + write!(stderr, "To fix: ").expect("Failed to write to stderr."); stderr .reset() .expect("Failed to reset stderr output color."); @@ -138,7 +142,7 @@ fn get_args_altered_for_extension_run( Ok(args) } -fn inner_main() -> DfxResult { +fn inner_main(log_level: &mut Option) -> DfxResult { let em = ExtensionManager::new(dfx_version())?; let installed_extension_manifests = em.load_installed_extension_manifests()?; let builtin_templates = builtin_templates(); @@ -154,6 +158,7 @@ fn inner_main() -> DfxResult { } let (verbose_level, log) = setup_logging(&cli_opts); + *log_level = Some(verbose_level); let identity = cli_opts.identity; let effective_canister_id = cli_opts.provisional_create_canister_effective_canister_id; @@ -171,10 +176,11 @@ fn inner_main() -> DfxResult { } fn main() { - let result = inner_main(); + let mut log_level: Option = None; + let result = inner_main(&mut log_level); if let Err(err) = result { let error_diagnosis = diagnose(&err); - print_error_and_diagnosis(err, error_diagnosis); + print_error_and_diagnosis(log_level, err, error_diagnosis); std::process::exit(255); } }