Feature/sentry.quartz - #5505
michaelmairegger wants to merge 35 commits into
Conversation
… dependencies and streamlining SentryCronJobListener options handling
# Conflicts: # .generated.NoMobile.sln # Sentry.sln
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
michaelmairegger
changed the title
…and update Quartz integration
|
@jamescrosswell I will continue watching Quartz.Net alpha releases and adopt it until final bits of 4.0 will be released. |
Fantastic, thank you @michaelmairegger ! Maybe we mark the PR as draft until then? That let's me know it doesn't yet need my attention (you can always tag me directly if you want my input on something before v4 is released). |
michaelmairegger
changed the title
… collection parameter and streamlining option configuration
…dSentryScope method, update SentryMetricsMiddleware to support configurable options
c05a9f7 to
59fdcd8
Compare
|
@jamescrosswell quartz.net released final bits of v4. I updated the package to the final version and would say this first version of the package is feature complete. |
michaelmairegger
changed the title
Nice - thanks @michaelmairegger. I wasn't expecting them to get v4 out so quickly! Apologies in advance... I might be a bis slow on reviewing - there's quite a but going on right now and the invention of AI has made it much easier for people to make PRs - so I'm spread a bit thin. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5505 +/- ##
==========================================
+ Coverage 74.72% 74.93% +0.21%
==========================================
Files 515 523 +8
Lines 18949 19061 +112
Branches 3696 3706 +10
==========================================
+ Hits 14159 14284 +125
+ Misses 3908 3890 -18
- Partials 882 887 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The format-code workflow can't push its auto-fix to a fork branch, so it fails the check instead. Applying the fix here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every package needs a name/packageUrl/mainDocsUrl entry so craft can create its registry entry on first publish; a missing `name` fails the whole registry target rather than just that package. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 688c054. Configure here.
|
|
||
| public async ValueTask Invoke(IJobExecutionContext context, JobExecutionDelegate next, CancellationToken cancellationToken) | ||
| { | ||
| using var _ = _sentryHub.PushScope(); |
There was a problem hiding this comment.
I like the idea of pushing a fresh scope for each job.
Maybe worth adding the job id as an attribute to the scope here... that way it's easy to match errors with the job they relate to?
There was a problem hiding this comment.
do you mean something like this?
using var _ = _sentryHub.PushScope();
_sentryHub.ConfigureScope(scope => scope.SetTag("quartz.job", context.JobDetail.Key.Name));
[...]There was a problem hiding this comment.
I haven't used Quartz for about 10 years but assuming context.JobDetail.Key.Name uniquely identifies either the job or the job run, yes, something like that 😄
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| public async ValueTask Invoke(IJobExecutionContext context, JobExecutionDelegate next, CancellationToken cancellationToken) | ||
| { | ||
| var jobType = context.JobInstance.GetType(); | ||
| var info = _sentryCronInformation.GetOrAdd(jobType, _ => new SentryCronInformation(context.JobInstance)); |
There was a problem hiding this comment.
I think this might be a mistake. _sentryCronInformation caches the jobs but currently the key used in the cache is the job type.
Quartz identifies a scheduled job by its JobKey (name + group), not by its class... which is what should be used as the cache key.
This has only worked until now since your sample happens to assign the class names as the job keys... but that's specific to your usage of Quartz in the sample application.
|
@jamescrosswell I will review the proposed changes next week and provide code changes then |
|
|
||
| public SentryCronJobMiddleware(IHub hub, IOptions<SentryCronJobOptions> options, ILogger<SentryCronJobMiddleware> logger) | ||
| { | ||
| _hub = hub; |
There was a problem hiding this comment.
Bug: The _sentryCronInformation dictionary in the singleton middleware never removes entries, causing a memory leak in applications that dynamically create jobs.
Severity: MEDIUM
Suggested Fix
Implement a cleanup mechanism for the _sentryCronInformation dictionary. This could involve using a cache with an eviction policy (e.g., MemoryCache with sliding expiration) or hooking into Quartz.NET's job lifecycle events to remove entries when a job is unscheduled or deleted. This will prevent the dictionary from growing indefinitely.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Sentry.Quartz/SentryCronJobMiddleware.cs#L16
Potential issue: The `SentryCronJobMiddleware` is a singleton that uses a
`ConcurrentDictionary` called `_sentryCronInformation` to cache information about jobs,
using the job key as the dictionary key. However, there is no mechanism to remove
entries from this dictionary. In long-running applications that dynamically create and
schedule jobs with unique keys, this dictionary will grow indefinitely, leading to a
memory leak. While applications with a fixed, static set of jobs are unaffected, those
with dynamic job creation will experience unbounded memory growth over time, which can
degrade performance and eventually cause the application to crash.
2 participants
This PR implements and showcases Quartz.Net integration for Sentry
Note: Quartz.Net v4 is still alpha, therefore we should wait until v4 is finally released.
fixes #4601