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

Possible bug if transaction is not sampled? #894

Closed
mfb opened this issue May 15, 2024 · 4 comments · Fixed by #898
Closed

Possible bug if transaction is not sampled? #894

mfb opened this issue May 15, 2024 · 4 comments · Fixed by #898
Assignees

Comments

@mfb
Copy link

mfb commented May 15, 2024

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:

        if (!$transaction->getSampled()) {
            return;
        }

        $this->transaction = $transaction;

        SentrySdk::getCurrentHub()->setSpan($this->transaction);

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.

@mfb
Copy link
Author

mfb commented May 15, 2024

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 $parentSpan && $parentSpan->getSampled()?

@cleptric
Copy link
Member

cleptric commented May 15, 2024

Yep, we're fixing this in the core, Laravel & Symfony SDKs.
We'll only keep the transaction around though, to keep memory pressure low.

@mfb
Copy link
Author

mfb commented May 15, 2024

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.

@mfb
Copy link
Author

mfb commented May 15, 2024

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.

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

Successfully merging a pull request may close this issue.

3 participants