Skip to content

Commit 58847ba

Browse files
committed
Merge branch 'gabe/update-logging' into gabe/trunk-14336-bail-early-on-non-retry-errors-from-the-api
2 parents 01b972b + 43ee91f commit 58847ba

File tree

9 files changed

+151
-32
lines changed

9 files changed

+151
-32
lines changed

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

api/src/client.rs

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,17 @@ pub struct ApiClient {
1919
org_url_slug: String,
2020
}
2121

22+
pub fn get_api_host() -> String {
23+
std::env::var(TRUNK_PUBLIC_API_ADDRESS_ENV)
24+
.ok()
25+
.and_then(|s| if s.is_empty() { None } else { Some(s) })
26+
.unwrap_or_else(|| DEFAULT_ORIGIN.to_string())
27+
}
28+
2229
impl ApiClient {
2330
const TRUNK_API_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(30);
31+
// This should always be fast
32+
const TRUNK_TELEMETRY_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(1);
2433
const TRUNK_API_TOKEN_HEADER: &'static str = "x-api-token";
2534

2635
pub fn new<T: AsRef<str>>(api_token: T, org_url_slug: T) -> anyhow::Result<Self> {
@@ -32,10 +41,7 @@ impl ApiClient {
3241
let api_token_header_value = HeaderValue::from_str(api_token)
3342
.map_err(|_| anyhow::Error::msg("Trunk API token is not ASCII"))?;
3443

35-
let api_host = std::env::var(TRUNK_PUBLIC_API_ADDRESS_ENV)
36-
.ok()
37-
.and_then(|s| if s.is_empty() { None } else { Some(s) })
38-
.unwrap_or_else(|| DEFAULT_ORIGIN.to_string());
44+
let api_host = get_api_host();
3945
tracing::debug!("Using public api address {}", api_host);
4046

4147
let telemetry_host = if api_host.contains("https://") {
@@ -72,7 +78,7 @@ impl ApiClient {
7278
.append(Self::TRUNK_API_TOKEN_HEADER, api_token_header_value);
7379

7480
let telemetry_client = Client::builder()
75-
.timeout(Self::TRUNK_API_TIMEOUT)
81+
.timeout(Self::TRUNK_TELEMETRY_TIMEOUT)
7682
.default_headers(telemetry_client_default_headers)
7783
.build()?;
7884

@@ -298,8 +304,6 @@ pub(crate) const NOT_FOUND_CONTEXT: &str = concat!(
298304
"(Settings -> Manage Organization -> Organization Slug).",
299305
);
300306

301-
const HELP_TEXT: &str = "\n\nFor more help, contact us at https://slack.trunk.io/";
302-
303307
pub(crate) fn status_code_help<T: FnMut(&Response) -> String>(
304308
response: Response,
305309
check_unauthorized: CheckUnauthorized,
@@ -311,11 +315,7 @@ pub(crate) fn status_code_help<T: FnMut(&Response) -> String>(
311315
let base_error_message = create_error_message(&response);
312316

313317
if !response.status().is_client_error() {
314-
response.error_for_status().map_err(|reqwest_error| {
315-
let error_message = format!("{base_error_message}{HELP_TEXT}");
316-
tracing::warn!("{}", error_message);
317-
anyhow::Error::from(reqwest_error)
318-
})
318+
response.error_for_status().map_err(anyhow::Error::from)
319319
} else {
320320
let domain: Option<String> = url::Url::parse(api_host)
321321
.ok()
@@ -339,11 +339,8 @@ pub(crate) fn status_code_help<T: FnMut(&Response) -> String>(
339339
_ => base_error_message,
340340
};
341341

342-
let error_message_with_help = format!("{error_message}{HELP_TEXT}");
343-
344-
tracing::warn!("{}", error_message_with_help);
345342
match response.error_for_status() {
346-
Ok(..) => Err(anyhow::Error::msg(error_message_with_help)),
343+
Ok(..) => Err(anyhow::Error::msg(error_message)),
347344
Err(error) => Err(anyhow::Error::from(error)),
348345
}
349346
}

cli/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ clap-verbosity-flag = "3.0.2"
5353
proto = { path = "../proto" }
5454
regex = "1.11.1"
5555
lazy_static = "1.5.0"
56+
url = "2.5.4"
5657

5758
[dev-dependencies]
5859
test_utils = { version = "0.1.0", path = "../test_utils" }

cli/src/context_quarantine.rs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use context::{
99
};
1010
use quick_junit::TestCaseStatus;
1111

12-
use crate::error_report::log_error;
12+
use crate::error_report::{log_error, Context};
1313

1414
#[derive(Debug, Default, Clone)]
1515
pub struct QuarantineContext {
@@ -184,7 +184,13 @@ pub async fn gather_quarantine_context(
184184
match api_client.get_quarantining_config(request).await {
185185
Ok(response) => response,
186186
Err(err) => {
187-
let error_code = log_error(&err, None);
187+
let error_code = log_error(
188+
&err,
189+
Context {
190+
base_message: Some("Unable to find quarantine config".into()),
191+
org_url_slug: request.org_url_slug.clone(),
192+
},
193+
);
188194
if error_code == exitcode::SOFTWARE {
189195
Err(err)?;
190196
}
@@ -258,7 +264,9 @@ pub async fn gather_quarantine_context(
258264
failures
259265
.iter()
260266
.for_each(|failure| log_failure(failure, request, api_client));
267+
tracing::info!("");
261268
}
269+
let quarantined_failure_count = quarantined_failures.len();
262270
quarantine_results.quarantine_results = quarantined_failures;
263271
quarantine_results.group_is_quarantined =
264272
quarantine_results.quarantine_results.len() == total_failures;
@@ -269,7 +277,15 @@ pub async fn gather_quarantine_context(
269277
tracing::info!("No failed tests to quarantine, returning exit code from command.");
270278
exit_code
271279
} else if !quarantine_results.group_is_quarantined {
272-
tracing::info!("Not all test failures were quarantined, returning exit code from command.");
280+
tracing::info!(
281+
"Quarantined {} out of {} test failures",
282+
quarantined_failure_count,
283+
total_failures
284+
);
285+
tracing::info!(
286+
"Not all test failures were quarantined, using exit code {} from command",
287+
exit_code
288+
);
273289
exit_code
274290
} else if exit_code != EXIT_SUCCESS {
275291
tracing::info!(
@@ -279,6 +295,7 @@ pub async fn gather_quarantine_context(
279295
} else {
280296
exit_code
281297
};
298+
tracing::info!("");
282299

283300
Ok(QuarantineContext {
284301
exit_code,

cli/src/error_report.rs

Lines changed: 76 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,79 @@
1+
use api::client::get_api_host;
12
use http::StatusCode;
23

3-
pub fn log_error(error: &anyhow::Error, base_message: Option<&str>) -> i32 {
4+
pub(crate) const UNAUTHORIZED_CONTEXT: &str = concat!(
5+
"Your Trunk organization URL slug or token may be incorrect - find it in the Trunk app",
6+
);
7+
8+
fn add_settings_url_to_context(
9+
base_message: String,
10+
domain: Option<String>,
11+
org_url_slug: &String,
12+
) -> String {
13+
match domain {
14+
Some(present_domain) => {
15+
let settings_url = format!(
16+
"{}/{}/settings",
17+
present_domain.replace("api", "app"),
18+
org_url_slug
19+
);
20+
format!(
21+
"{}\n Hint: Your settings page can be found at: {}",
22+
base_message, settings_url
23+
)
24+
}
25+
None => base_message,
26+
}
27+
}
28+
29+
const HELP_TEXT: &str = "\n\nFor more help, contact us at https://slack.trunk.io/";
30+
31+
pub struct Context {
32+
pub base_message: Option<String>,
33+
pub org_url_slug: String,
34+
}
35+
36+
pub fn log_error(error: &anyhow::Error, context: Context) -> i32 {
437
let root_cause = error.root_cause();
38+
let Context {
39+
base_message,
40+
org_url_slug,
41+
} = context;
542
if let Some(io_error) = root_cause.downcast_ref::<std::io::Error>() {
643
if io_error.kind() == std::io::ErrorKind::ConnectionRefused {
744
tracing::warn!(
845
"{}",
9-
message(base_message, "could not connect to trunk's server")
46+
message(base_message, "Unable to connect to trunk's server")
1047
);
1148
return exitcode::OK;
1249
}
1350
}
1451

52+
let api_host = get_api_host();
1553
if let Some(reqwest_error) = root_cause.downcast_ref::<reqwest::Error>() {
1654
if let Some(status) = reqwest_error.status() {
1755
if status == StatusCode::UNAUTHORIZED
1856
|| status == StatusCode::FORBIDDEN
1957
|| status == StatusCode::NOT_FOUND
2058
{
21-
tracing::warn!("{}", message(base_message, "unauthorized to access trunk"),);
59+
tracing::warn!(
60+
"{}",
61+
add_settings_url_to_context(
62+
message(base_message, UNAUTHORIZED_CONTEXT),
63+
Some(api_host),
64+
&org_url_slug
65+
)
66+
);
2267
return exitcode::SOFTWARE;
2368
}
2469
}
2570
}
2671

27-
tracing::error!("{}", message(base_message, "error"));
72+
if let Some(base_message) = base_message {
73+
tracing::error!("{}", message(Some(base_message), HELP_TEXT));
74+
} else {
75+
tracing::error!("{}", error);
76+
}
2877
tracing::error!(hidden_in_console = true, "Caused by error: {:#?}", error);
2978
exitcode::SOFTWARE
3079
}
@@ -45,9 +94,30 @@ pub fn error_reason(error: &anyhow::Error) -> String {
4594
"unknown".into()
4695
}
4796

48-
fn message(base_message: Option<&str>, hint: &str) -> String {
97+
fn message(base_message: Option<String>, hint: &str) -> String {
4998
match base_message {
50-
Some(base_message) => format!("{} because {}", base_message, hint),
99+
Some(base_message) => format!("{}\n\t{}", base_message, hint),
51100
None => String::from(hint),
52101
}
53102
}
103+
104+
#[test]
105+
fn adds_settings_if_domain_present() {
106+
let host = "https://api.fake-trunk.io";
107+
let final_context = add_settings_url_to_context(
108+
"base_context".into(),
109+
Some(host.into()),
110+
&String::from("fake-org-slug"),
111+
);
112+
assert_eq!(
113+
final_context,
114+
"base_context\n Hint: Your settings page can be found at: https://app.fake-trunk.io/fake-org-slug/settings",
115+
)
116+
}
117+
118+
#[test]
119+
fn does_not_add_settings_if_domain_absent() {
120+
let final_context =
121+
add_settings_url_to_context("base_context".into(), None, &String::from("fake-org-slug"));
122+
assert_eq!(final_context, "base_context",)
123+
}

cli/src/main.rs

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use third_party::sentry;
66
use tracing_subscriber::{filter::FilterFn, prelude::*};
77
use trunk_analytics_cli::{
88
context::gather_debug_props,
9-
error_report::log_error,
9+
error_report::{log_error, Context},
1010
quarantine_command::{run_quarantine, QuarantineArgs},
1111
test_command::{run_test, TestArgs},
1212
upload_command::{run_upload, UploadArgs, UploadRunResult},
@@ -99,6 +99,7 @@ fn main() -> anyhow::Result<()> {
9999
.build()?
100100
.block_on(async {
101101
let cli = Cli::parse();
102+
let org_url_slug = cli.org_url_slug();
102103
let log_level_filter = cli.verbose.log_level_filter();
103104
setup_logger(
104105
log_level_filter,
@@ -114,8 +115,22 @@ fn main() -> anyhow::Result<()> {
114115
match run(cli).await {
115116
Ok(exit_code) => std::process::exit(exit_code),
116117
Err(error) => {
117-
let exit_code = log_error(&error, None);
118+
let exit_code = log_error(
119+
&error,
120+
Context {
121+
base_message: None,
122+
org_url_slug,
123+
},
124+
);
118125
guard.flush(None);
126+
if exit_code != exitcode::OK {
127+
tracing::error!("Unable to proceed, returning exit code: {}", exit_code);
128+
} else {
129+
tracing::error!(
130+
"Errors occurred during upload, returning default exit code: {}",
131+
exit_code
132+
);
133+
}
119134
std::process::exit(exit_code);
120135
}
121136
}

cli/src/quarantine_command.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use clap::Args;
22

33
use crate::{
4-
error_report::log_error,
4+
error_report::{log_error, Context},
55
upload_command::{run_upload, UploadArgs, UploadRunResult},
66
};
77

@@ -27,14 +27,21 @@ impl QuarantineArgs {
2727

2828
// This is an alias to `run_upload`, but does not exit on upload failure
2929
pub async fn run_quarantine(QuarantineArgs { upload_args }: QuarantineArgs) -> anyhow::Result<i32> {
30+
let org_url_slug = upload_args.org_url_slug.clone();
3031
let upload_run_result = run_upload(upload_args, None, None).await;
3132
upload_run_result.map(
3233
|UploadRunResult {
3334
exit_code,
3435
upload_bundle_error,
3536
}| {
3637
if let Some(e) = upload_bundle_error {
37-
log_error(&e, Some("Error uploading test results"));
38+
log_error(
39+
&e,
40+
Context {
41+
base_message: Some("Error uploading test results".into()),
42+
org_url_slug,
43+
},
44+
);
3845
}
3946
exit_code
4047
},

cli/src/test_command.rs

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use constants::EXIT_FAILURE;
99

1010
use crate::{
1111
context::{gather_debug_props, gather_pre_test_context},
12-
error_report::log_error,
12+
error_report::{log_error, Context},
1313
upload_command::{run_upload, UploadArgs, UploadRunResult},
1414
};
1515

@@ -69,6 +69,7 @@ pub async fn run_test(
6969
test_run_result.exec_start = None;
7070
}
7171

72+
let org_url_slug = upload_args.org_url_slug.clone();
7273
let upload_run_result =
7374
run_upload(upload_args, Some(pre_test_context), Some(test_run_result)).await;
7475

@@ -85,7 +86,13 @@ pub async fn run_test(
8586
},
8687
)
8788
.or_else(|e| {
88-
log_error(&e, Some("Error uploading test results"));
89+
log_error(
90+
&e,
91+
Context {
92+
base_message: Some("Error uploading test results".into()),
93+
org_url_slug,
94+
},
95+
);
8996
Ok(test_run_result_exit_code)
9097
})
9198
}

cli/src/upload_command.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ pub async fn run_upload(
211211
.await?;
212212

213213
let upload_started_at = chrono::Utc::now();
214+
tracing::info!("Uploading test results...");
214215
let upload_bundle_result = upload_bundle(
215216
meta.clone(),
216217
&api_client,
@@ -252,6 +253,9 @@ pub async fn run_upload(
252253
tracing::debug!("Failed to send telemetry: {:?}", e);
253254
}
254255

256+
if upload_bundle_result.is_err() {
257+
tracing::error!("Failed to upload bundle");
258+
}
255259
Ok(UploadRunResult {
256260
exit_code,
257261
upload_bundle_error: upload_bundle_result.err(),
@@ -281,7 +285,7 @@ async fn upload_bundle(
281285
.put_bundle_to_s3(&upload.url, &bundle_temp_file)
282286
.await?;
283287
if exit_code == EXIT_SUCCESS {
284-
tracing::info!("Done");
288+
tracing::info!("Upload successful");
285289
} else {
286290
tracing::info!(
287291
"Upload successful; returning unsuccessful exit code of test run: {}",

0 commit comments

Comments
 (0)