sentry-php

sentry-php copied to clipboard

Add support for Dynamic Sampling

We decided not to access the request internally and will instead rely on people passing in a correctly populated TransactionContext to be able to use Dynamic Sampling. This is in line with the implementation of the propagation of sentry-trace.

For our Laravel and Symfony SDKs, we can then set this up automatically.

Additional, the following required items for DS were added :

• The TransactionSource https://develop.sentry.dev/sdk/event-payloads/transaction/#transaction-annotations
• The user_segment https://develop.sentry.dev/sdk/event-payloads/user/#attributes & https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#envelope-header
• An adequately populated envelope header https://develop.sentry.dev/sdk/performance/dynamic-sampling-context/#envelope-header

Usage example:

use Sentry\Tracing\TransactionContext;
use function Sentry\startTransaction;

$sentryTraceHeader = $request->getHeaderLine('sentry-trace');
$baggageHeader = $request->getHeaderLine('baggage');

$transactionContext = TransactionContext::continueFromHeaders($sentryTraceHeader, $baggageHeader);

...
startTransaction($transactionContext);

Co-authored-by: Alex Bouma

Sure thing, we can split it up once I'm done. Right now, this is more of a dumping ground so CI runs on all PHP versions 😄

Thanks for the feedback everyone. I'm planning to merge this tomorrow.

@ste93cry Next time, please don’t update my branches without consulting me first. You are always welcome to branch off from my branch and propose your changes in a separate PR.

While I appreciate the updates to the test suite, I’ll have to revert your last commit partially, as my team and I agree on not adding further deprecation warnings to the code base. We added an @deprecated annotation, we will point this out more clearly in the changelog and will cover this as well in the migration guide for v4.0.

We added an @deprecated annotation, we will point this out more clearly in the changelog and will cover this as well in the migration guide for v4.0.

People don't care about docblocks, they don't even see them in their day-to-day work and there is no way for them to know that something got deprecated if they don't look specifically at each doblock each time. @trigger_error is the standard way nowadays in PHP projects to let them know that some method is gonna be deprecated and removed.

as my team and I agree on not adding further deprecation warnings to the code base

I would love to hear the reasons behind this choice. Furthermore, I would like to add that, while we're at it, we're all in the same team, and so if such a decision was taken, at least it should have been communicated to the most active contributors of this project :sweat_smile:

Nothing is wrong if people continue using TransactionContext::fromSentryTrace, if they don't want to use Dynamic Sampling. We will likely release this before Symfony and Laravel, so I rather not add deprecation warnings to people's apps, without a way for them to fix it.

We discussed this in our daily, the team is @getsentry/team-web-sdk-backend.

Having just a @deprecated annotation gets a thumbs up from me, as static analysis tools can easily pick up on these annotations and warn the developer. A deprecation could always be "escalated" later on by adding trigger_error() call, when it's more actionable.

I wonder how many notifications https://github.com/deprecated gets because of @deprecated :D