Fix: Ignore DiagnosticSource Integration if no Sampling available.#1238
Fix: Ignore DiagnosticSource Integration if no Sampling available.#1238lucas-zimerman merged 15 commits intomainfrom
Conversation
lucas-zimerman
changed the title
Codecov Report
@@ Coverage Diff @@
## main #1238 +/- ##
==========================================
+ Coverage 81.55% 81.57% +0.02%
==========================================
Files 212 212
Lines 6971 7024 +53
Branches 1454 1463 +9
==========================================
+ Hits 5685 5730 +45
- Misses 838 846 +8
Partials 448 448
Continue to review full report at Codecov.
|
test (triggering github actions)
josh-degraw
left a comment
josh-degraw
left a comment
There was a problem hiding this comment.
Were the errant spans that were being created only happening for the SentryEFCoreListener and the SentrySqlListener?
src/Sentry.DiagnosticSource/Internals/DiagnosticSource/SentryEFCoreListener.cs
Outdated
Show resolved
Hide resolved
Yes |
…tps://github.com/getsentry/sentry-dotnet into fix/ignore-diagnosticsource-with-no-transactions
|
As discussed here https://discord.com/channels/621778831602221064/621792783316942878/896041457264259103 |
|
@lucas-zimerman i took the liberty of handling the merge conflict on the pr. But this looks good to me. A couple of minor comments above, but nothing that should block this one |
src/Sentry.DiagnosticSource/Internals/DiagnosticSource/SentryEFCoreListener.cs
Outdated
Show resolved
Hide resolved
test/Sentry.DiagnosticSource.Tests/Integration/SQLite/SentryDiagnosticListenerTests.cs
Outdated
Show resolved
Hide resolved
…tps://github.com/getsentry/sentry-dotnet into fix/ignore-diagnosticsource-with-no-transactions
lucas-zimerman
deleted the
fix/ignore-diagnosticsource-with-no-transactions
branch
| _diagnosticListener = DiagnosticListener.AllListeners.Subscribe(_subscriber); | ||
| if (options.TracesSampleRate == 0) | ||
| { | ||
| options.DiagnosticLogger?.Log(SentryLevel.Info, "DiagnosticSource Integration is now disabled due to TracesSampleRate being set to zero."); |
There was a problem hiding this comment.
Prefer LogInfo instead of Log(Level.Info
| if (options.TracesSampleRate == 0) | ||
| { | ||
| options.DiagnosticLogger?.Log(SentryLevel.Info, "DiagnosticSource Integration is now disabled due to TracesSampleRate being set to zero."); | ||
| options.DisableDiagnosticSourceIntegration(); |
There was a problem hiding this comment.
THe integration instance was already called, why mutate options after the fact to remove it?
It's probably best to remove this line
5 participants
This PR solves two use cases.
The first issue is solved by disabling the integration during the application's startup if found that TracesSampleRate is set to zero.
The latter is fixed by not trying to retrieve/create a span if the current transaction is not being sampled.