Antithesis instrumentation #1049
Conversation
THardy98
force-pushed
the
antithesis-instrumentation
branch
3 times, most recently
from
7215c4d to
1b9db17
Compare
| x => { | ||
| crate::antithesis::assert_always!( | ||
| false, | ||
| "activity cancel yields supported command", | ||
| ::serde_json::json!({ | ||
| "activity_seq": self.shared_state.attrs.seq, | ||
| "unexpected_command": format!("{x:?}"), | ||
| "state": self.state().to_string(), | ||
| }) | ||
| ); | ||
| panic!("Invalid cancel event response {x:?}"); |
There was a problem hiding this comment.
I don't think we need these since a normal panic is still going to trigger antithesis - but the extra info can be useful.
It might make sense to make a macro like dbg_panic that these three places can use, but, not a big deal.
crates/sdk-core/src/abstractions.rs
Outdated
| #[cfg(not(feature = "antithesis_assertions"))] | ||
| macro_rules! dbg_panic { |
There was a problem hiding this comment.
We don't need two versions of this. You can just feature flag the assert_always statement
crates/sdk-core/src/antithesis.rs
Outdated
| /// No-op when feature is disabled. | ||
| #[inline(always)] | ||
| pub(crate) fn ensure_init() {} |
There was a problem hiding this comment.
This doesn't need to exist because no one calls it when compiled without this flag.
In fact I'm surprised it doesn't warn as dead code
crates/sdk-core/src/antithesis.rs
Outdated
| ($condition:expr, $message:literal, $details:expr) => {{ | ||
| let _ = ($condition, $message); | ||
| let _ = $details; | ||
| }}; | ||
| ($condition:expr, $message:literal) => {{ | ||
| let _ = ($condition, $message); |
There was a problem hiding this comment.
I think assigning things like this is actually going to cause the compiler not to optimize some stuff away.
Honestly, I'm not sure we want to do this technique at all. It's slightly more wordy, but guarding every call to assert_always with the flag at the callsite is safer in terms of actually making sure everything gets compiled out
|
@Sushisource just FYI, I've been holding off on this given that the Antithesis experiment finishes next week and we're blocked on upgrading core due to worker heartbeating anyways (also doesn't seem worth to cherry-pick this commit into SDKs). It's more useful to investigate what we're getting already with the Antithesis + omes runs as of right now. Depending on the success of the experiment, i'll either address + merge or just close the PR. |
I ended up just doing it anyways. Antithesis errors are difficult to debug. I want to try it on a test Python branch with this cherry-picked to see if we can get something more focused. |
…e machines instead of direct calls to antithesis assert_always (dbg_panic just logs in non-release builds with no antithesis_assertions feature flag anyways)
THardy98
force-pushed
the
antithesis-instrumentation
branch
from
1bbaa43 to
ada2e54
Compare
| return Err(fatal!("{}", | ||
| "StartChildWorkflowExecutionInitiated attributes were unset".to_string(), | ||
| )); |
There was a problem hiding this comment.
All these ones are oddly formatted with {} just being replaced w/ a static string
What was changed
Add
antithesis_assertionsfeature flag to core SDK. Minimal assertions included, but can be added with more usage.Why?
antithesis testing
It's not really, just compiles