Skip to content

Commit 2741f7b

Browse files
committed
feat(uptime): remove built-in 2xx status code check
Relies on assertions to validate responses instead of a hard-coded 2xx check. When no assertion is configured, any response is considered a success. Callers are expected to configure a status code assertion to validate the response code. Fixed [NEW-758](https://linear.app/getsentry/issue/NEW-758/remove-built-in-status-code-check-from-uptime-checker)
1 parent 0a3c263 commit 2741f7b

3 files changed

Lines changed: 22 additions & 29 deletions

File tree

src/checker.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ fn make_trace_header(config: &CheckConfig, trace_id: &Uuid, span_id: SpanId) ->
4444
/// which the check is being made.
4545
pub trait Checker: Send + Sync {
4646
/// Makes a request to a url to determine whether it is up.
47-
/// Up is defined as returning a 2xx within a specific timeframe.
47+
/// Up is defined as responding within a specific timeframe, optionally validated by assertions.
4848
fn check_url(
4949
&self,
5050
check: &ScheduledCheck,

src/checker/reqwest_checker.rs

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -334,18 +334,11 @@ fn to_check_result(
334334
max_assertion_ops,
335335
region,
336336
)
337-
} else if r.status().is_success() {
338-
Check::success()
339337
} else {
340-
Check::code_failure(r.status())
338+
Check::success()
341339
}
342340
} else {
343-
// TODO: rust 2024 allows let-chaining, so the enclosing if-statement can be
344-
// folded into the the if let
345-
match r.status().is_success() {
346-
true => Check::success(),
347-
false => Check::code_failure(r.status()),
348-
}
341+
Check::success()
349342
}
350343
}
351344
Err(e) => Check::other_failure(e.into()),
@@ -510,7 +503,7 @@ fn to_errored_request_infos(
510503

511504
impl Checker for ReqwestChecker {
512505
/// Makes a request to a url to determine whether it is up.
513-
/// Up is defined as returning a 2xx within a specific timeframe, along with executing
506+
/// Up is defined as responding within a specific timeframe, along with passing
514507
/// an optional user-defined assert that is specified by the user which can use
515508
/// the result json, status code, and response header values.
516509
#[tracing::instrument]
@@ -889,7 +882,7 @@ mod tests {
889882
}
890883

891884
#[tokio::test]
892-
async fn test_simple_400() {
885+
async fn test_simple_400_no_assertion() {
893886
let server = MockServer::start();
894887
let checker = ReqwestChecker::new_internal(
895888
Options {
@@ -916,14 +909,13 @@ mod tests {
916909
let check = ScheduledCheck::new_for_test(tick, config);
917910
let result = checker.check_url(&check, "us-west").await;
918911

919-
assert_eq!(result.status, CheckStatus::Failure);
912+
// Without an assertion, a non-2xx response is still considered a success.
913+
// Callers are expected to configure a status code assertion to validate the response.
914+
assert_eq!(result.status, CheckStatus::Success);
920915
assert_eq!(
921916
result.request_info.and_then(|i| i.http_status_code),
922917
Some(400)
923918
);
924-
let reason = result.status_reason.unwrap();
925-
assert_eq!(reason.status_type, CheckStatusReasonType::Failure);
926-
assert_eq!(reason.description, "Got non 2xx status: 400 Bad Request");
927919

928920
head_mock.assert();
929921
}
@@ -953,6 +945,13 @@ mod tests {
953945

954946
let config = CheckConfig {
955947
url: server.url("/error").to_string(),
948+
assertion: crate::assertions::Assertion {
949+
root: crate::assertions::Op::StatusCodeCheck {
950+
value: 500,
951+
operator: crate::assertions::Comparison::NotEqual,
952+
},
953+
}
954+
.into(),
956955
..Default::default()
957956
};
958957

@@ -1007,6 +1006,13 @@ mod tests {
10071006

10081007
let config = CheckConfig {
10091008
url: server.url("/error").to_string(),
1009+
assertion: crate::assertions::Assertion {
1010+
root: crate::assertions::Op::StatusCodeCheck {
1011+
value: 500,
1012+
operator: crate::assertions::Comparison::NotEqual,
1013+
},
1014+
}
1015+
.into(),
10101016
..Default::default()
10111017
};
10121018

src/types/result.rs

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ use crate::assertions::compiled;
22
use crate::assertions::compiled::EvalPath;
33
use crate::assertions::Assertion;
44
use chrono::{DateTime, TimeDelta, Utc};
5-
use http::StatusCode;
65
use hyper::rt::ConnectionStats;
76
use hyper::stats::AbsoluteDuration;
87
use hyper::stats::RequestStats;
@@ -414,18 +413,6 @@ impl Check {
414413
}
415414
}
416415

417-
pub fn code_failure(status: StatusCode) -> Self {
418-
Self {
419-
result: CheckStatus::Failure,
420-
reason: Some(CheckStatusReason {
421-
status_type: CheckStatusReasonType::Failure,
422-
description: format!("Got non 2xx status: {status}"),
423-
details: None,
424-
}),
425-
assert_path: None,
426-
}
427-
}
428-
429416
pub fn other_failure(reason: CheckStatusReason) -> Self {
430417
Self {
431418
result: CheckStatus::Failure,

0 commit comments

Comments
 (0)