Document where/when the Sample decision should be made

[marandaneto]

Right now sampling happens on startTransaction for Java, .NET, Python, and probably the others too.

The problem I see here is, a transaction.status could be set at a later point if an error has occurred.

I'd like to use the status to properly sample my transactions, I'd like higher sampling if there's an error.
This is not possible right now as it's being sampled or not already in the startTransaction and status might not be available, yet.

A possible solution would be to sample only on finish when the transaction should have its full payload.

cc @rhcarvalho @bruno-garcia @Tyrrrz @maciejwalkowiak @HazAT @brustolin

[rhcarvalho]

I wrote about this today in getsentry/sentry-go#320 (comment).

What you're asking for is not possible with head-based sampling.

You can, however, overwrite the sampling decision for the current transaction at any point before it goes to Sentry (that is, you can drop the transaction), but you cannot expect that it will affect peer services, because at that point sampling decisions were already propagated via HTTP headers.

A possible solution would be to sample only on finish when the transaction should have its full payload.

This is not possible in our current model, as, while the transaction is ongoing, all outgoing HTTP requests would have no decision to propagate.

[Tyrrrz]

You can, however, overwrite the sampling decision for the current transaction at any point before it goes to Sentry (that is, you can drop the transaction), but you cannot expect that it will affect peer services, because at that point sampling decisions were already propagated via HTTP headers.

Can you please clarify how a user would do that? Is there a way to drop a transaction manually somehow?

Is it possible to configure sampling in such way that the transaction is dropped AFTER the app sends a request to an external service?

[marandaneto]

@rhcarvalho I see, got it, thanks.

You can, however, overwrite the sampling decision for the current transaction at any point before it goes to Sentry (that is, you can drop the transaction), but you cannot expect that it will affect peer services, because at that point sampling decisions were already propagated via HTTP headers.

I'd ask the same as @Tyrrrz , are you mentioning a sort of beforeSend? on Java, transactions don't go over the event pipeline.

[rhcarvalho]

Can you please clarify how a user would do that?

In languages that don't impose limitations on what programmers can do, having a reference to a span or transaction allows them to change any aspect of it.

Is there a way to drop a transaction manually somehow?

Yes, with a traces sampler or with event processors, or overriding the sampling decision as above. In languages that do provide for making fields private, etc, I think as long as users can address their use cases we don't need to make everything public and mutable.

Is it possible to configure sampling in such way that the transaction is dropped AFTER the app sends a request to an external service?

Event processors or overriding sample decision (again, in some languages there's nothing we can do about it, but we don't have to support it everywhere else -- "Java is not Python" and vice versa).

[marandaneto]

ok so event processors don't run against transactions (Java at least), I know there are differences across SDKs about that, I'm just not sure which one is the right approach cus I don't know the trade-offs yet.

Also, on Java, right now only isSampled is public but not setSampled so both hold not possible.

[Tyrrrz]

same, event processors also don't run in .NET.
I believe @bruno-garcia was strongly against it.

[bruno-garcia]

Some background to that decision:
JS runs EventProcessor and that was unexpected at the time.

When building Java and .NET we decided to not to do the same so we can control what runs for each captured Transaction instead of all our integrations, user defined event processor etc, that had errors in mind (that by definition happen less often), suddenly start getting this new event type.

The idea was that we'll be faced with trade offs such as instrumentation cost (as in CPU, memory etc) and richness of the transaction in Sentry. So instead of adding everything OOTB from v1, we could go with a more lightweight instrumentation and add things as we see the trade off of the instrumentation is worth the performance hit (i.e: how much I/O are we really allowed to do on Android before users stop using our Performance instrumentation?).

[marandaneto]

Some background to that decision:
JS runs EventProcessor and that was unexpected at the time.

When building Java and .NET we decided to not to do the same so we can control what runs for each captured Transaction instead of all our integrations, user defined event processor etc, that had errors in mind (that by definition happen less often), suddenly start getting this new event type.

The idea was that we'll be faced with trade offs such as instrumentation cost (as in CPU, memory etc) and richness of the transaction in Sentry. So instead of adding everything OOTB from v1, we could go with a more lightweight instrumentation and add things as we see the trade off of the instrumentation is worth the performance hit (i.e: how much I/O are we really allowed to do on Android before users stop using our Performance instrumentation?).

thanks, which I generally agree, some stuff would consume quite a bit eg checking storage size, available memory etc, a lot of IO.

I thought setSampled was totally private but it's hidden under getSpanContext instead so one could still do:
transaction.getSpanContext().setSampled(...)
which is a workaround but as @bruno-garcia said, if once we start debugging things, till we find out that a transaction was propagated but dropped before being sent, could be hard.

+1 to drop and think this thoroughly, better/easier adding than removing later, wdyt?

[marandaneto]

btw https://develop.sentry.dev/sdk/performance/#sdk-configuration describes how it works

[marandaneto]

@Tyrrrz @maciejwalkowiak @brustolin the decision was to hide setSampled if possible (depending on language features).
sampling is head-based only for now thru TransactionContext, tracesSampleRate, or TracesSamplerCallback