Skip to content
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

Threads mistakenly attach to main thread's span #674

Open
Ten0 opened this issue Jul 25, 2024 · 6 comments
Open

Threads mistakenly attach to main thread's span #674

Ten0 opened this issue Jul 25, 2024 · 6 comments

Comments

@Ten0
Copy link
Contributor

Ten0 commented Jul 25, 2024

The first time we call sentry-related functions in a thread, it attaches to whichever Span the main thread is currently attached to.
This causes very wrong sub-spanning (which I just spend 6 hours investigating).
Attaching to sentry client, DSN, parameters..., etc seems correct, but span definitely not.

I feel like maybe the Scope should be a property that's not inherited from whichever scope the main thread happens to be in at that moment, because if there is a sub-thread it's very possible that the main thread is doing unrelated work at that moment, and that unrelated work may very well involve its own spans and data, so it seems that it's not correct to attach to those.

Related issues: #507
Related writeup: https://swatinem.de/blog/log-contexts/

@Ten0

This comment has been minimized.

@Ten0

This comment has been minimized.

@Ten0

This comment has been minimized.

@Ten0
Copy link
Contributor Author

Ten0 commented Jul 25, 2024

Ok so my understanding is that there's no way to prevent the Hub to attach to the current scope of the main thread.
It looks like the "main Hub" shouldn't really be attached to any thread, and even the main thread should have its own Hub.
That would fix this issue.

@szokeasaurusrex
Copy link
Member

Hi @Ten0, I am a bit confused by your most recent comment. Is this issue resolved, or does something still need to be fixed?

If something needs to be fixed, please explain in more detail exactly what is going wrong. A reproduction would be extremely helpful.

@Ten0
Copy link
Contributor Author

Ten0 commented Dec 13, 2024

What's going wrong is in the scenario where I do:

  1. Init sentry on main thread
  2. Spawn a sub-thread
  3. Keep doing work on main thread
  4. sub-thread calls any sentry-related function

Then the sub-thread attaches to whatever scope the main thread was running at that exact moment.
You end up with wrong sub-spanning, that is, things showing as sub-spans of unrelated other things that the main thread is working on.
That is irrelevant, wrong in the general case (above) and hard to debug.

The reason why this happens is that there's a "main hub" that gets attached to whichever thread is currently running when the first Sentry-related function initializes, and then stays linked to it and shared with other things that get attached to it.

The fix would be to have the main hub, whose state would be described by whatever we put in init, and modifiable explicitly, not be attached to any thread, so that the thread that first calls sentry-related stuff doesn't have a special and unreliable behavior of things implicitly attaching to it.

My current workaround for this behavior is this:

pub fn init_sentry(
	before_send: Option<BeforeCallback>,
) -> std::result::Result<sentry::ClientInitGuard, std::env::VarError> {
	// Initialize sentry on a separate thread to workaround:
	// https://github.com/getsentry/sentry-rust/issues/674#issuecomment-2250386287
	// (So that sub-threads don't bind to random scopes the main thread may be using at any given time)
	std::thread::spawn(|| {
		use std::env::var;
		let options = sentry::ClientOptions {
			dsn: Some(var("SentryDsn")?.parse().expect("Invalid value for sentry DSN")),
			release: Some(Cow::Owned(var("CommitShaShort")?)),
			environment: Some(Cow::Owned(
				var("SENTRY_OVERRIDE_ENVIRONMENT_NAME")
					.ok()
					.map_or_else(|| var("SCD_GROUP"), Ok)?,
			)),
			before_send,
			..sentry::ClientOptions::default()
		};
		std::env::set_var("RUST_BACKTRACE", "1");
		let guard = sentry::init(options);

		// Make sure that we are the main hub, so that any thread will bind to our hub and have the client,
		// otherwise Sentries wouldn't be sent
		if !std::sync::Arc::ptr_eq(&sentry::Hub::current(), &sentry::Hub::main()) {
			panic!("Sentry-related functions should never be called before initializing Sentry");
		}

		#[cfg(feature = "tracing")]
		tracing_integration::init();

		Ok(guard)
	})
	.join()
	.expect("Sentry initialization thread failed")
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants