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

panic because nil client in DynamicSamplingContextFromTransaction #518

Closed
eliecharra opened this issue Dec 19, 2022 · 3 comments · Fixed by #520
Closed

panic because nil client in DynamicSamplingContextFromTransaction #518

eliecharra opened this issue Dec 19, 2022 · 3 comments · Fixed by #520

Comments

@eliecharra
Copy link

Summary

In a context where we do not have initialized sentry, when finishing a transaction that have Sampled = sentry.SampledFalse will trigger a panic. I have a piece of code that conditionally force the sampling of a given transaction, this is working well but if I run my unit tests on the function that panics.

In general, I think the SDK should not panic if sentry is not configured.

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x635330]

goroutine 1 [running]:
github.com/getsentry/sentry-go.DynamicSamplingContextFromTransaction(0xc00015c000)
	/tmp/gopath1614006584/pkg/mod/github.com/getsentry/[email protected]/dynamic_sampling_context.go:46 +0xb0
github.com/getsentry/sentry-go.(*Span).toEvent(0xc00015c000)
	/tmp/gopath1614006584/pkg/mod/github.com/getsentry/[email protected]/tracing.go:405 +0x2e5
github.com/getsentry/sentry-go.(*Span).Finish(0xc00015c000)
	/tmp/gopath1614006584/pkg/mod/github.com/getsentry/[email protected]/tracing.go:173 +0xb5

Steps To Reproduce

https://go.dev/play/p/vzesJ4l9TJa

Expected Behavior

No panic on Finish()

SDK

  • sentry-go version: v0.16.0
  • Go version: 1.19
  • Using Go Modules? yes

Sentry

  • Using hosted Sentry in sentry.io? yes

Happy to shoot a PR if you quickly explain how would you like to handle that 🙏🏻

@tonyo
Copy link
Contributor

tonyo commented Dec 20, 2022

Thanks for the report @eliecharra, can confirm that it's crashing, which is totally undesirable.

We'll look into this (most likely won't happen in December, though), but in the meantime, could you perhaps initialize the Sentry SDK with empty DSN in your tests as a workaround?
E.g. something like

sentry.Init(sentry.ClientOptions{})

@eliecharra
Copy link
Author

We'll look into this (most likely won't happen in December, though)

Sure sure no worries this is not a blocker at all 👍🏻 Also the workaround is perfectly acceptable so far 🙏🏻

Thanks for your work.

@cleptric
Copy link
Member

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

Successfully merging a pull request may close this issue.

3 participants