-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
Possible bug if transaction is not sampled? #894
Comments
I guess this was done as an optimization? But, to keep the optimization in place, while indicating that the transaction is not sampled, maybe other tracing code could check if both |
Yep, we're fixing this in the core, Laravel & Symfony SDKs. |
Ok so I'm not crazy :p Not sure if core PHP SDK needs to be fixed at all, seemed like mainly an issue in Laravel thus I filed the issue here. |
Actually, looking at core PHP SDK, the GuzzleTracingMiddleware needs some logic fixes: First of all, it looks like breadcrumb data is only set if there is a span, but I think breadcrumb data should always be set? And then, don't start span if $span exists but is not sampled. |
How do you use Sentry?
Sentry SaaS (sentry.io)
SDK version
Sentry PHP 4.7.0
Steps to reproduce
I might be confused or misunderstanding something, but in looking at Sentry\Laravel\Tracing\Middleware, the transaction is only set on the hub if it was sampled:
However, code in the PHP SDK, such as the getTraceparent(), getW3CTraceparent() and getBaggage() functions, looks at the span on the hub to see if it was sampled. Therefore, I would have assumed that we want to put a transaction that is not sampled on the hub.
Is the Laravel SDK "doing it wrong" and it should be putting even unsampled transactions on the hub? Or am I just confused and misunderstanding things, which is quite possible :)
Expected result
I think an unsampled transaction should be on the hub, so that it can be determined that is was not sampled.
Actual result
The sampling decision, which was actually false, appears to be null.
The text was updated successfully, but these errors were encountered: