-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
Propose a new way to continue a trace #14
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
* Start Date: 2022-09-26 | ||
* RFC Type: feature | ||
* RFC PR: https://github.com/getsentry/rfcs/pull/14 | ||
|
||
# Summary | ||
|
||
This RFC proposes a new way to continue a trace when creating nested transactions. | ||
|
||
# Motivation | ||
|
||
The current way we propagate `sentry-trace` and `baggage`, is to pass a correctly populated `TransactionContext` as the first argument to `startTransaction()`. | ||
|
||
```php | ||
use Sentry\Tracing\TransactionContext; | ||
use function Sentry\startTransaction; | ||
|
||
$transactionContext = TransactionContext::fromHeaders($sentryTraceHeader, $baggageHeader); | ||
$transaction = startTransaction($transactionContext); | ||
|
||
``` | ||
|
||
In case someone starts another nested transaction without passing in any context, a new trace will be started and the Dynamic Sampling Context is lost as well. | ||
Using transactions inside transactions was a workaround as the span summary view was not available back then. | ||
|
||
# Options Considered | ||
|
||
## Add TransactionContext::fromParent() | ||
|
||
```php | ||
use Sentry\Tracing\TransactionContext; | ||
use function Sentry\startTransaction; | ||
|
||
$transactionContext = TransactionContext::fromParent($transaction); | ||
$transaction = startTransaction($transactionContext); | ||
|
||
public static function fromParent(Transaction $transaction) | ||
{ | ||
$context = new self(); | ||
$context->traceId = $transaction->getTraceId(); | ||
$context->parentSpanId = $transaction->getParentSpanId(); | ||
$context->sampled = $transaction->getSampled(); | ||
$context->getMetadata()->setBaggage($transaction->getBaggage()); | ||
|
||
return $context; | ||
} | ||
``` | ||
|
||
## Add a third argument to `startTransaction()` | ||
|
||
```php | ||
use Sentry\Tracing\TransactionContext; | ||
use function Sentry\startTransaction; | ||
|
||
$transactionContext = new TransactionContext(); | ||
$transaction = startTransaction($transactionContext, [], bool $continueTrace = true); | ||
``` | ||
|
||
Inside `TransactionContext::__contruct`, we could check for an ongoing transaction on the Hub and continue the trace automatically. | ||
|
||
# Drawbacks/Impact | ||
|
||
- This will increase the public API surface of our SDKs | ||
- Depending on the option, it's either more complex or more magical. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd opt for exposing the option but giving it a reasonable default so that most people don't have to deal with it. That said, I'd echo Manoel's question: In what circumstances is starting a new transaction in the same process better than just nesting spans? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #14 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, yes, thanks for that. But I'm still not sure if the issue is what this fixes or the fact that maybe we really shouldn't be nesting transactions... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would only work if you have access to the
transaction
instance, would you do that rather than adding spans to the running transaction?Usually, continuing trace is used for distributed tracing, such as frontend <-> servers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this RFC was a use-case in getsentry/sentry itself. There are cases where we start transactions inside transactions.
Link to our [internal Slack conversation].(https://sentry.slack.com/archives/C043MQLSLTS/p1663598491050169)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness, these are the patterns of usage that currently break dynamic sampling.
https://github.com/getsentry/sentry/blob/a10312cf1f10af0a79a07b9895565963429dce24/src/sentry/ingest/ingest_consumer.py#L156-L168