-
-
Notifications
You must be signed in to change notification settings - Fork 222
UnsampledTransactions to reduce memory pressure #4212
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
Conversation
…-dotnet into unsampled-transactions2
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.
I couldn't spot a test that validates we're still sending the trace-id downstream from http requests for example. That's one concern I have, if we have an unsampled transaction, the trace id should still be used to link all data leaving the SDK (errors, user feedback, replay) as well as all outgoing HTTP request should have that trace id.
// report to sentry is the span count (in the client report), SDK users may refer to things like | ||
// `ITransaction.Spans.Count`, so we create an actual collection |
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.
what's the use case for users to get count of spans from transactions?
the goal is to deprecate transactions in the long term. Spans are expected to stream out of the SDK
Anything done absed on span count is going to break so I wouldn't optimzie for that
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.
It's for the client report. At the moment the sampling is done at the transaction level... so if a transaction gets sampled out we're sampling out the transaction + it's tree of child spans. We report the number of spans we sampled out in the client report.
/// avoid lots of unecessary processing. The only thing we need to track is the number of spans that would have been | ||
/// created (the client reports detailing discarded events includes this detail). | ||
/// </summary> | ||
internal sealed class UnsampledTransaction : NoOpTransaction |
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.
I was hoping we'd get away without having to create an instance for each transaction but I also dunno how we'd deal with trace propagation in that case. And since most of the allocation are on the spans anyway this prob going to shave off enough GC pressure
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.
Yeah, especially since we have to propagate things like the SampleRand, we need separate instances of UnsampledTransaction
.
As well as not having to allocate anything for child spans, the UnsampledTransaction
class itself is quite a bit lighter than TransactionTracer
(no Contexts, Breadcrumbs, Extras etc.). It should make a big difference in things like high volume ASP.NET Core apps.
Good call. I added some tests to the Hub since the Baggage and Trace headers that get propagated get read off the currently Active span in the Hub: sentry-dotnet/src/Sentry/SentryMessageHandler.cs Lines 136 to 154 in a46bce2
|
Added a light weight
UnsampledTransaction
class to be used when transactions are not sampled. The aim is to reduce memory pressure when sampling less than 100% of transactions.Resolves #3636:
Transaction.NoOp
to easy memory pressure #3636Replaces #3972: