Feature/sentry.quartz - #5505
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
18ff2fe to
8e9c712
Compare
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.
There was a problem hiding this comment.
I think the key metrics (like job duration) Sentry infers/tracks automatically from the checkins. So you get things like this in the job monitoring dashboard:
Are there some things missing from the build in dashboards that prompted you to add the metrics middleware?
Everything we have to build to get those default dashboard visualisations should be covered in the developer docs for check ins.
There was a problem hiding this comment.
I am not seeing this screen. I am using self hosted v26.8.0 and there the view does not have any duration chart. This is all I see. Therefore I started writing the SentryMetricsMiddleware.
There was a problem hiding this comment.
Hm, sounds like there might be an issue with your self hosted setup. I asked the Sentry docs about this and they came back with:
In general, self-hosted Sentry has near-feature-parity with SaaS for cron monitoring — Crons is included in the default "Feature Complete" self-hosted deployment.
Regarding the missing average duration issue your user is seeing, there are a few known causes worth investigating:
- Known Bug: Missing Duration on Check-ins
There is a known issue where cron monitors show an "invalid duration" error or display — for duration when no duration parameter is included in the check-in request. This has been reported on SaaS as well.#105192 - Invalid Duration Issue
- Ingestion Pipeline Issues
If a check-in's in_progress event is dropped (e.g., due to an ingestion outage or network issue), the resulting check-in will have no duration because the start time was never recorded. The OK check-in then creates a check-in with no corresponding in_progress, so duration cannot be calculated.#59607 - Missed Check-in Issue
- Self-Hosted: Missing ingest-monitors Consumer
On older self-hosted setups, if the ingest-monitors Kafka consumer is not running, check-ins may not be processed correctly at all. This was a known configuration gap before Crons went GA.
- Latency / Tight Timeout Margins
If a job's completion check-in arrives very close to the max runtime threshold, queue processing latency can cause it to be marked as timed out before the completion is registered — resulting in missing duration data.
You could always add this SentryMetricsMiddleware to your own project if you need it but we shouldn't add it in the Sentry.Quartz integration.
There was a problem hiding this comment.
I see everything you mention above, just the chart with duration is missing. The recent check-ins table is just cropped in the screenshot, but it has valid data.
Neither docker nor when opening the dev tools in the browser shows any error when loading the page. So I think that there is a slight feature diff between SaaS and self-hosted. As far as I can remember, some time ago, there was a chart showing the average duration.
For me there is no problem to remove this middleware from code and use it in my projects only.
There was a problem hiding this comment.
I'm not too sure why the duration isn't showing - that would be a question for support as it isn't controlled anywhere in SDKs like sentry-dotnet.
Let's remove the Metrics middleware from the SDK. We don't want to duplicate information that is already there for most people.
|
|
||
| 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.
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