search

getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
599 stars 206 forks source link

Add ability to set sampling rate for the DiagnosticSource integrations #3611

Open ericsampson opened 2 months ago

ericsampson commented 2 months ago

Problem Statement

The DiagnosticSource auto-instrumentation of EF and SqlClient is client, but they create an absolute TON of Spans.

To the point where I have to set the overall sampling strategy of the SDK to a very low value in order to avoid costing a untenable $$$$ in Performance credits. But this also has a very unfortunate side-effect of discarding important low-volume traces that we need to see. And it's not a good solution to expect users to manually set up complicated and brittle sampling rules to capture these traces. ("investigation mode" helps, but it's not ideal)

Solution Brainstorm

MVP: I would really love to be able to set sampling rates for:

Phase 2: be able set the individual sampling rates for every span type (EF and SqlClient db.connection, db.query, etc)

Phase 3: if customer feedback supports the need, then allow people to set custom sampling callbacks for the above.

Thanks a ton!!!

(it's possible that I could work on contributing the MVP over time, if someone could point me in the right direction re code to steal/reuse)

jamescrosswell commented 2 months ago

Hey @ericsampson, thanks for the suggestion!

How would you see this working with the end to end trace? A SentryTransaction can include all kinds of spans. Some of them might be DB spans and others might represent other operations. If you just filtered out the DB spans, you'd presumably end up with a partial trace that might be a bit hard to make sense of.

In terms of a solution, with the current APIs, this is a bit tricky since sampling actions get applied to Transactions (not Spans) and even using SetBeforeSendTransaction there's no way to remove specific spans (the Transaction.Spans collection is readonly). There is a plan to get rid of Transactions and everything that gets sent through to Sentry will just be Spans at that point... so potentially this is something we could support in the future, if it makes sense for SDK users.

ericsampson commented 2 months ago

If you just filtered out the DB spans, you'd presumably end up with a partial trace that might be a bit hard to make sense of.

I mean it would be better than the potential alternative of disabling the integration entirely in order to reduce the volume of generated spans to a more acceptable level. At least with this feature, there would be a percentage of traces with full details to look at when desired, and still be able to capture much more (or all) of the total app transactions.

jamescrosswell commented 2 months ago

Hm, what are the "important low-volume traces" that you need to see? Do those share any defining characteristics that would make them easy to identify?

If so then you could use SentryOptions.TracesSampler to set a different sample rate for those traces than the others.