-
-
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: Crons support (Hangfire) #3128
Conversation
|
d6f5f32
to
200b8b0
Compare
|
||
public class SecondJob | ||
{ | ||
[SentryMonitorSlug("job-that-throws")] |
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.
can we use the method name by default? so adding this attribute becomes optional?
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.
That would cause the SDK to send a CheckIn
with every Hangfire job. Is this something we'd be okay with?
internal SentryServerFilter(IHub hub, IDiagnosticLogger? logger = null) | ||
{ | ||
_hub = hub; | ||
_logger = logger ?? _hub.GetSentryOptions()?.DiagnosticLogger; |
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.
when do we not get a logger? seems YAGNI?
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<PropertyGroup> | ||
<TargetFrameworks>net8.0;net6.0</TargetFrameworks> |
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.
Not testing the .NET Framework build?
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 forgot to add the explainer: Hangfire is not strong naming their assemblies and CI chokes on Windows on net48. Due to the viral nature and the use of InternalsVisibleTo
I could not get it to opt out.
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.
Only thing I'd consider 'blocking' is agreeing if we need 2 public APIs instead of only 1 (with 3 arguments)
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.
LGTM - just a couple of comments.
…otnet into feat/hangfire
Resolves #2239
This PR accomplishes the following things:
SentrySdk.CaptureCheckIn
API for manual captureThe Hangfire integration works as follows:
It adds an extension to the hangfire config allowing the SDK to add its filter for all jobs.
For the integration to work it requires a
monitor slug
that needs to be present. The monitor slug can be added via our own HangfireAttribute on the job running in the background.with no changes required to the call into Hangfire