rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Propose a new way to continue a trace

Open cleptric opened this issue 3 years ago • 8 comments

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

Rendered RFC

Refs https://github.com/getsentry/develop/issues/690

cleptric avatar Sep 26 '22 13:09 cleptric

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

Swatinem avatar Oct 03 '22 08:10 Swatinem

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 avatar Oct 03 '22 12:10 sl0thentr0py

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.

sl0thentr0py avatar Oct 03 '22 12:10 sl0thentr0py

A similar issue that would be solved by a solution to continue the trace thru the same process. https://github.com/getsentry/sentry-dart/issues/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).

marandaneto avatar Oct 11 '22 12:10 marandaneto

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.

cleptric avatar Oct 11 '22 15:10 cleptric

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 https://github.com/getsentry/sentry/issues/43647

mitsuhiko avatar Jan 26 '23 22:01 mitsuhiko

@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?

cleptric avatar Jan 30 '23 18:01 cleptric

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

  1. we continue a trace independent of the transaction (eg: is that what happens if we continue from a cronjob?)
  2. 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()

mitsuhiko avatar Jan 30 '23 22:01 mitsuhiko