-
-
Notifications
You must be signed in to change notification settings - Fork 205
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
feat: blocking detection #2709
Merged
Merged
feat: blocking detection #2709
Changes from 11 commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
5a29e10
feat: blocking detection
bruno-garcia 17bd0bf
half assed stuff from the weekend
bruno-garcia a25993c
Merge branch 'main' into feat/blocking-detector
bruno-garcia 7173711
verify
bruno-garcia ef951da
Merge branch 'main' into feat/blocking-detector
bruno-garcia e1173fa
ref
bruno-garcia 579fc98
Merge remote-tracking branch 'origin' into feat/blocking-detector
bruno-garcia 43515be
binding
bruno-garcia d83362b
context
bruno-garcia 047c0dc
wip
bruno-garcia fc1d708
wip
bruno-garcia 869168f
Merge branch 'main' into feat/blocking-detector
jamescrosswell 9859b21
Fixed compiler errors
jamescrosswell 3fa8364
Update CHANGELOG.md
jamescrosswell 3cb5bb6
Format code
getsentry-bot 50f5b6d
Merge branch 'feat/blocking-detector' of github.com:getsentry/sentry-…
jamescrosswell 85509fd
Reintroducing Bruno's changes with tests
jamescrosswell c75eb29
Reintroduced changes to MainSentryEventProcessor
jamescrosswell 0c15b10
Reintroduced the core blocking capability
jamescrosswell 0c855f4
Update SentryStackTraceFactory.cs
jamescrosswell 663658f
Merge branch 'main' into feat/blocking-detector
jamescrosswell dcd5680
Update HubTests.CaptureEvent_ActiveTransaction_UnhandledExceptionTran…
jamescrosswell 669d3b9
Added blocking detection suppression
jamescrosswell 39fb85c
Merge branch 'feat/blocking-detector' of github.com:getsentry/sentry-…
jamescrosswell 3d74dde
Format code
getsentry-bot c549b92
Merge branch 'main' into feat/blocking-detector
jamescrosswell 679e099
Reverted unintentional changes to ApiDefinitions
jamescrosswell 48d186b
Merge branch 'main' into feat/blocking-detector
jamescrosswell a00a4a5
Merge branch 'feat/blocking-detector' of github.com:getsentry/sentry-…
jamescrosswell e2ac986
Fixed stack traces for blocking calls
jamescrosswell 578532e
Merge branch 'main' into feat/blocking-detector
jamescrosswell c593f82
Update BlockingMonitor.cs
jamescrosswell 4d5b8e1
Some basic tests for BlockingMonitor
jamescrosswell fb5602f
Added code to ensure blocking events only get sent once per day
jamescrosswell b315b1a
Format code
getsentry-bot 39926f4
Added System.Diagnostics.Metrics for blocking calls so users can know…
jamescrosswell 81e1951
removed throttling mechanism
jamescrosswell 4e732f4
Update CHANGELOG.md
jamescrosswell 2e8b05a
Merge branch 'main' into feat/blocking-detector
bruno-garcia df2a8d6
merge issues
bruno-garcia 42ed4a8
Refactored to improve testability
jamescrosswell c87472e
Merge branch 'feat/blocking-detector' of github.com:getsentry/sentry-…
jamescrosswell 1cf1d18
Format code
getsentry-bot 45c67b9
Moved IBlockingMonitor to a separate file
jamescrosswell f8d872a
Merge branch 'feat/blocking-detector' of github.com:getsentry/sentry-…
jamescrosswell 9846d5a
Merge branch 'main' into feat/blocking-detector
jamescrosswell 322afff
Merge branch 'main' into feat/blocking-detector
jamescrosswell 80be4dd
Merge branch 'main' into feat/blocking-detector
jamescrosswell ffef982
Applied review feedback
jamescrosswell File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
Sample usages of the Sentry SDK for ASP.NET Core on an MVC app | ||
|
||
Start by changing the DSN in `appsettings.json`, `Sentry:Dsn` property with your own. | ||
No DSN yet? Get one for free at https://sentry.io/ to give this sample a run. | ||
|
||
Blocking detection: | ||
* It's turned on via option `CaptureBlockingCalls` | ||
* In the `HomeController` there's an action that causes a blocking call on an async method. | ||
* You can trigger it with: | ||
* `GET http://localhost:5001/home/block/true` | ||
|
||
Results `Was blocking? True` and an event captured in Sentry. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
using Sentry.Internal; | ||
using Sentry.Protocol; | ||
|
||
// Namespace starting with Sentry makes sure the SDK cuts frames off before reporting | ||
namespace Sentry.Ben.Diagnostics | ||
{ | ||
internal class BlockingMonitor(Func<IHub> getHub, SentryOptions options) | ||
{ | ||
[ThreadStatic] | ||
private static int t_recursionCount; | ||
|
||
public void BlockingStart(DetectionSource detectionSource) | ||
{ | ||
if (!Thread.CurrentThread.IsThreadPoolThread) | ||
{ | ||
return; | ||
} | ||
|
||
t_recursionCount++; | ||
|
||
try | ||
{ | ||
if (t_recursionCount == 1) | ||
{ | ||
var evt = new SentryEvent | ||
{ | ||
Level = SentryLevel.Warning, | ||
Message = | ||
"Blocking method has been invoked and blocked, this can lead to ThreadPool starvation. Learn more about it: " + | ||
"https://learn.microsoft.com/en-us/aspnet/core/fundamentals/best-practices#avoid-blocking-calls ", | ||
// SentryThreads = new []{new SentryThread | ||
// { | ||
// Id = Environment.CurrentManagedThreadId, | ||
// }}, | ||
SentryExceptions = new[] | ||
{ | ||
new SentryException | ||
{ | ||
ThreadId = Environment.CurrentManagedThreadId, | ||
Mechanism = new Mechanism | ||
{ | ||
Type = "BlockingCallDetector", | ||
Handled = false, | ||
Description = "Blocking calls can cause ThreadPool starvation." | ||
}, | ||
Type = "Blocking call detected", | ||
Stacktrace = DebugStackTrace.Create( | ||
options, | ||
new StackTrace(true), | ||
true, | ||
// Skip frames once the Sentry frames are already removed | ||
// Originally this skipped 3 more frames (context ? 3 : 6) but since Sentry hides the System frames | ||
// by default, this will focus on the App frame but allow the user to see a bit deeper | ||
// into what .NET does for blocking. | ||
detectionSource == DetectionSource.SynchronizationContext ? 0 : 3), | ||
} | ||
}, | ||
}; | ||
|
||
// TODO: How to render in the UI a better "suggested fix"? | ||
evt.SetExtra( | ||
"suggestion", | ||
"Analyzer to warn you from blocking calls on async flows; https://www.nuget.org/packages/Microsoft.VisualStudio.Threading.Analyzers/"); | ||
|
||
getHub().CaptureEvent(evt); | ||
} | ||
} | ||
catch | ||
{ | ||
} | ||
} | ||
|
||
public void BlockingEnd() | ||
{ | ||
if (!Thread.CurrentThread.IsThreadPoolThread) | ||
{ | ||
return; | ||
} | ||
|
||
t_recursionCount--; | ||
} | ||
} | ||
|
||
internal enum DetectionSource | ||
{ | ||
SynchronizationContext, | ||
EventListener | ||
} | ||
} |
50 changes: 50 additions & 0 deletions
50
src/Sentry/Ben.BlockingDetector/DetectBlockingSynchronizationContext.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Namespace starting with Sentry makes sure the SDK cuts frames off before reporting | ||
namespace Sentry.Ben.Diagnostics | ||
{ | ||
// Tips of the Toub | ||
internal sealed class DetectBlockingSynchronizationContext : SynchronizationContext | ||
{ | ||
private readonly BlockingMonitor _monitor; | ||
private readonly SynchronizationContext? _syncCtx; | ||
|
||
public DetectBlockingSynchronizationContext(BlockingMonitor monitor) | ||
{ | ||
_monitor = monitor; | ||
|
||
SetWaitNotificationRequired(); | ||
} | ||
|
||
public DetectBlockingSynchronizationContext(BlockingMonitor monitor, SynchronizationContext? syncCtx) | ||
: this(monitor) | ||
=> _syncCtx = syncCtx; | ||
|
||
public override int Wait(IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout) | ||
{ | ||
if (millisecondsTimeout == 0) | ||
{ | ||
return WaitInternal(waitHandles, waitAll, millisecondsTimeout); | ||
} | ||
|
||
_monitor.BlockingStart(DetectionSource.SynchronizationContext); | ||
|
||
try | ||
{ | ||
return WaitInternal(waitHandles, waitAll, millisecondsTimeout); | ||
} | ||
finally | ||
{ | ||
_monitor.BlockingEnd(); | ||
} | ||
} | ||
|
||
private int WaitInternal(IntPtr[] waitHandles, bool waitAll, int millisecondsTimeout) | ||
{ | ||
if (_syncCtx != null) | ||
{ | ||
return _syncCtx.Wait(waitHandles, waitAll, millisecondsTimeout); | ||
} | ||
|
||
return base.Wait(waitHandles, waitAll, millisecondsTimeout); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@bruno-garcia won't the SynchronizationContext in ASP.NET Core always be null? Is this code here just in case SDK users have implemented a custom sync context for some reason?