-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix tracing spans #99
Conversation
This is definitely better but not sure it's quite right if the test code uses spans itself. I wrote this test: use shuttle::{thread, check_random};
use shuttle::sync::{Arc, Mutex};
use tracing::{warn_span, warn};
use test_log::test;
#[test]
fn tracing_nested_spans() {
check_random(|| {
let lock = Arc::new(Mutex::new(0));
let threads: Vec<_> = (0..3).map(|i| {
let lock = lock.clone();
thread::spawn(move || {
let outer = warn_span!("outer", tid=i);
let _outer = outer.enter();
{
let mut locked = lock.lock().unwrap();
let inner = warn_span!("inner", tid=i);
let _inner = inner.enter();
warn!("incrementing from {}", *locked);
*locked += 1;
}
})
}).collect();
for thread in threads {
thread.join().unwrap();
}
}, 10)
} and in the trace output, all the
I think we probably have to do something smarter than just exiting the top span every context switch. Since we know spans form a tree, maybe we do something like remember the entire path below some known sentinel span, exit all those at a context switch, and re-enter them when we switch back? All of Shuttle's spans are above any application spans in the tree, so this should work unless we end up violating that property. Testing wise, you could just write your own mock |
WIP, pushing now in as I will be out next week, and others can have a look/do stuff with it if they want. Did another attempt at it. This one is based on how they do it in Loom. Seems to be better than the previous version, but may still have issues. Have not tested it for futures yet. Left a couple of TODOs for where I will pick back up. |
Still haven't tested whether it gives the right output for futures, but it should hopefully not crash when they are involved anymore. |
@@ -227,3 +227,36 @@ fn iterate_over_hash_set(num_entries: u64) { | |||
fn hashset_without_set_seed() { | |||
check_uncontrolled_nondeterminism(|| iterate_over_hash_set(10), 1000); | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 230-262 is going to be removed.
Hmm so there seems to be no issues with futures. However, the scheme is not correct. What I believe we should be seeing (run outside of Shuttle):
And what we are seeing:
(lacking outer{tid=0}) So we seem to be seeing the same traces as Loom, but neither are displaying the correct traces. I'll look at this more once I am back. Sorry for the noise. |
Cleaned up the proposed fix. I want to invoke the power of frugality and suggest to merge this (after eventual comments). It is not perfect, but it is way way better than the current version of spans, which is pretty much useless. I want to spend time to fix it properly sometime in the future, but I think my time currently is better spent elsewhere. |
Of course, if someone else want to take a look at it, then go ahead. |
Added an issue for further fix here: #104 |
* Fix tracing spans * Address issues with previous attempt to fix the tracing spans * Make the tracing fix not cause crashes for futures * Cleanup the tracing span fix * Address comments from @jamesbornholt
I guess this is easiest described by showing the output. Two lines of output with
RUST_LOG=trace
turned on before this change:and after:
As for testing: I did start making the test which I used during development into a test where everything was automatic by turning on tracing and then capturing the output. This turned out to be quite hacky and required pulling in dependencies, so I decided to abandon it. If this code should break for some reason, then that does not pose a correctness issue, so no big need to have a test.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.