Propose a new way to continue a trace #14

cleptric · 2022-09-26T13:31:10Z

This RFC proposes a new way to continue a trace when creating nested transactions.

marandaneto · 2022-09-29T15:32:40Z

text/0014-continue-traces.md

+use Sentry\Tracing\TransactionContext;
+use function Sentry\startTransaction;
+
+$transactionContext = TransactionContext::continueFromHeaders($sentryTraceHeader, $baggageHeader);


On Java, it's a bit different but similar https://docs.sentry.io/platforms/android/performance/instrumentation/custom-instrumentation/#distributed-tracing
We've not implemented the continueFromHeaders method, reason is here.

marandaneto · 2022-09-29T15:34:25Z

text/0014-continue-traces.md

+use Sentry\Tracing\TransactionContext;
+use function Sentry\startTransaction;
+
+$transactionContext = TransactionContext::fromParent($transaction);


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.

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)

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

lobsterkatie · 2022-09-30T02:27:43Z

text/0014-continue-traces.md

+# Drawbacks/Impact
+
+- This will increase the public API surface of our SDKs
+- Depending on the option, it's either more complex or more magical.


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?

See #14 (comment)

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...

Swatinem · 2022-10-03T08:28:06Z

👍🏻 for the general idea. I implemented TransactionContext::continue_from_span in Rust since the very beginning since it felt like the right thing to do.

👎🏻 on a bool option that looks up the current Hub. Unless something changed, we modeled Transactions to be mostly independent from Hubs. There could be multiple concurrent transactions, and the decision which one is bound to the hub is up to the API user.

sl0thentr0py · 2022-10-03T12:08:18Z

I would ask @beezz, @untitaker etc to also maybe clarify the use case and if this is a workaround for some other problem because it is not entirely clear to many of us why starting a new transaction is necessary instead of just adding spans.
https://github.com/getsentry/sentry/blob/a10312cf1f10af0a79a07b9895565963429dce24/src/sentry/ingest/ingest_consumer.py#L156-L168

sl0thentr0py · 2022-10-03T12:20:58Z

Regarding preference, I agree with @Swatinem that a new method with explicit semantics is better than another magic bool variable that breaks hub assumptions on the current one.
The tradeoff here is that it won't magically make dynamic sampling work on those who use the current explicit startTransaction, they'll have to migrate their usages first.

marandaneto · 2022-10-11T12:45:48Z

A similar issue that would be solved by a solution to continue the trace thru the same process.
getsentry/sentry-dart#721
It works with the current workaround but this is semantically wrong because when you think about header, you think about external services (HTTP and so on).

cleptric · 2022-10-11T15:52:51Z

Thanks again for all your input! 🙂
I'm closing this for the moment and we can revisit it if the need for this will arise again in the future.

mitsuhiko · 2023-01-26T22:30:00Z

Can we revisit this? We ran into this again with monitors where we need to continue a trace (eg: create transaction context) but not yet create a transaction which I believe part of this RFC covers.

Refs getsentry/sentry#43647

cleptric · 2023-01-30T18:00:19Z

@mitsuhiko After talking with @evanpurkhiser and @davidenwang, it seems like this would be a different ask than the initial goal of this RFC.
What we had in mind here was to auto-magically continue a trace from startTransaction, the other use-case now being to propagate the trace without being in the context of a transaction.
Does this make sense?

mitsuhiko · 2023-01-30T22:44:04Z

@cleptric we can probably separate these things down but there are two paths we might want to look at:

we continue a trace independent of the transaction (eg: is that what happens if we continue from a cronjob?)
we do always continue a transaction, but create a clearer user experience around making a transaction when there is already one ongoing. (is that what we want?)

We can open a new RFC with a clearer set goal but unless we have an initial hunch where we are leaning here we might want to understand this first.

# Does this create a transaction or a trace. What are the consequences of either?
sentry_sdk.continue_trace_from_environment()

cleptric force-pushed the continue-trace branch from 6ead2c6 to 48c5fc7 Compare September 26, 2022 13:31

Propose a new way to continue a trace

5f9650b

cleptric force-pushed the continue-trace branch from 48c5fc7 to 5f9650b Compare September 26, 2022 13:32

cleptric self-assigned this Sep 26, 2022

cleptric linked an issue Sep 26, 2022 that may be closed by this pull request

[DEV-DOCS] start_transaction(true) getsentry/develop#690

Closed

cleptric requested review from sl0thentr0py, AbhiPrasad and mitsuhiko and removed request for AbhiPrasad September 27, 2022 11:35

sl0thentr0py requested review from AbhiPrasad and lobsterkatie September 27, 2022 11:45

lforst self-requested a review September 27, 2022 12:55

mitsuhiko mentioned this pull request Sep 28, 2022

Propose a Change to the Browser Trace Model #20

Closed

cleptric added 2 commits September 29, 2022 15:37

Remove HTTP option

9b55122

Add Drawbacks/Impact

92c9d09

marandaneto reviewed Sep 29, 2022

View reviewed changes

lobsterkatie reviewed Sep 30, 2022

View reviewed changes

cleptric added 2 commits September 30, 2022 13:54

Correct method name

cc6a66b

Clarify use case

54620cf

sl0thentr0py requested review from beezz and untitaker October 3, 2022 12:08

cleptric closed this Oct 11, 2022

chadwhitacre deleted the continue-trace branch January 19, 2023 15:45

mitsuhiko restored the continue-trace branch January 26, 2023 22:27

mitsuhiko reopened this Jan 26, 2023

sl0thentr0py removed their request for review August 1, 2023 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propose a new way to continue a trace #14

Propose a new way to continue a trace #14

cleptric commented Sep 26, 2022 •

edited

marandaneto Sep 29, 2022

marandaneto Sep 29, 2022 •

edited

cleptric Sep 30, 2022

sl0thentr0py Oct 3, 2022

lobsterkatie Sep 30, 2022

cleptric Sep 30, 2022

lobsterkatie Oct 4, 2022

Swatinem commented Oct 3, 2022

sl0thentr0py commented Oct 3, 2022

sl0thentr0py commented Oct 3, 2022

marandaneto commented Oct 11, 2022

cleptric commented Oct 11, 2022

mitsuhiko commented Jan 26, 2023 •

edited

cleptric commented Jan 30, 2023

mitsuhiko commented Jan 30, 2023

Propose a new way to continue a trace #14

Are you sure you want to change the base?

Propose a new way to continue a trace #14

Conversation

cleptric commented Sep 26, 2022 • edited

marandaneto Sep 29, 2022

Choose a reason for hiding this comment

marandaneto Sep 29, 2022 • edited

Choose a reason for hiding this comment

cleptric Sep 30, 2022

Choose a reason for hiding this comment

sl0thentr0py Oct 3, 2022

Choose a reason for hiding this comment

lobsterkatie Sep 30, 2022

Choose a reason for hiding this comment

cleptric Sep 30, 2022

Choose a reason for hiding this comment

lobsterkatie Oct 4, 2022

Choose a reason for hiding this comment

Swatinem commented Oct 3, 2022

sl0thentr0py commented Oct 3, 2022

sl0thentr0py commented Oct 3, 2022

marandaneto commented Oct 11, 2022

cleptric commented Oct 11, 2022

mitsuhiko commented Jan 26, 2023 • edited

cleptric commented Jan 30, 2023

mitsuhiko commented Jan 30, 2023

cleptric commented Sep 26, 2022 •

edited

marandaneto Sep 29, 2022 •

edited

mitsuhiko commented Jan 26, 2023 •

edited