3 ms·

>If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same. If the `.unwrap()` was replaced with

by sznio 11mo ago

>If they'd written explicit code to fail the request when that happened, the end result would have been exactly the same. If the `.unwrap()` was replaced with `.expect("Feature config is too large!")` it would certainly make the outage shorter.

YetAnotherNick 11mo ago
In general for unexpected errors like these the internal function should log the error, and I assume it was either logged or they can quickly deduce reason based on the line number.
Ygg2 11mo ago
> If the `.unwrap()` was replaced with `.expect("Feature config is too large!")` it would certainly make the outage shorter. It wouldn't, not meaningfully. The outage was caused by change in how they processed the queries. They had no way to observe the changes, nor canaries to see that change is killing them. Plus, they would still need to manually feed and restart services that ingested bad configs. `expect` would shave a few minutes; you would still spend hours figuring out and fixing it. Granted, using expect is better, but it's not a silver bullet.
- antihero 11mo ago
  A billion alerts in DD/Sentry/whatever saying the exact problem that coincide with the exact graph of failures would probably be helpful if someone looked at them.
  - Ygg2 11mo ago
    Not if everyone decides to 's/.unwrap()/.expect("Shouldn't happen")/g' in the code base. Or the good old: let x = match res { Ok(x) => x, Err(_) => unreachable!(), }
echelon 11mo ago
If the `.unwrap()` was replaced with 1: ? 2: map_err, or, or_else, etc. 3: match ... { Ok(..) => {}, Err(..) => {}, } 4: if let ... { } Then it would have been idiomatic Rust code and wouldn't have failed at all. The function signature returned a `Result<(), (ErrorFlags, i32)>` Seems like it should have returned an Err((ErrorFlags, i32)) here. Case 2 or 3 above would have done nicely. Removing unwrap() from Rust would have forced the proper handling of the function call and would have prevented this. Unwrap() is Rust's original sin.

by sznio 11mo ago

YetAnotherNick 11mo ago

In general for unexpected errors like these the internal function should log the error, and I assume it was either logged or they can quickly deduce reason based on the line number.

Ygg2 11mo ago

> If the `.unwrap()` was replaced with `.expect("Feature config is too large!")` it would certainly make the outage shorter. It wouldn't, not meaningfully. The outage was caused by change in how they processed the queries. They had no way to observe the changes, nor canaries to see that change is killing them. Plus, they would still need to manually feed and restart services that ingested bad configs. `expect` would shave a few minutes; you would still spend hours figuring out and fixing it. Granted, using expect is better, but it's not a silver bullet.

antihero 11mo ago
A billion alerts in DD/Sentry/whatever saying the exact problem that coincide with the exact graph of failures would probably be helpful if someone looked at them.
- Ygg2 11mo ago
  Not if everyone decides to 's/.unwrap()/.expect("Shouldn't happen")/g' in the code base. Or the good old: let x = match res { Ok(x) => x, Err(_) => unreachable!(), }

echelon 11mo ago

If the `.unwrap()` was replaced with 1: ? 2: map_err, or, or_else, etc. 3: match ... { Ok(..) => {}, Err(..) => {}, } 4: if let ... { } Then it would have been idiomatic Rust code and wouldn't have failed at all. The function signature returned a `Result<(), (ErrorFlags, i32)>` Seems like it should have returned an Err((ErrorFlags, i32)) here. Case 2 or 3 above would have done nicely. Removing unwrap() from Rust would have forced the proper handling of the function call and would have prevented this. Unwrap() is Rust's original sin.