-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Support check-ins for Quartz #2940
Conversation
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
- Support check-ins for Quartz ([#2940](https://github.com/getsentry/sentry-java/pull/2940)) If none of the above apply, you can opt out of this check by adding |
Performance metrics 🚀
|
Codecov ReportPatch has no changes to coverable lines.
📢 Thoughts on this report? Let us know!. |
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 think we should change the way we handle the monitor config, as described in the comment. Otherwise LGTM
@Nullable MonitorScheduleUnit timeUnit = null; | ||
|
||
try { | ||
List<? extends Trigger> triggersOfJob = |
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.
Shouldn't we use context.getTrigger()
instead?
Maybe we'd also have to add some part of the trigger to the monitor slug.
Because the we way it is now, if we have two or more triggers for the job it will always take the info of the last CronTrigger
in the triggersOfJob
list.
Small example (add to SentryDemoApplication in jakarta):
@Bean
public JobDetailFactoryBean jobDetail() {
JobDetailFactoryBean jobDetailFactory = new JobDetailFactoryBean();
jobDetailFactory.setName("hello there 123");
jobDetailFactory.setJobClass(SampleJob.class);
jobDetailFactory.setDescription("Invoke Sample Job service...");
jobDetailFactory.setDurability(true);
return jobDetailFactory;
}
@Bean
public SimpleTriggerFactoryBean trigger(JobDetail job) {
SimpleTriggerFactoryBean trigger = new SimpleTriggerFactoryBean();
trigger.setJobDetail(job);
trigger.setRepeatInterval(10000);
trigger.setRepeatCount(SimpleTrigger.REPEAT_INDEFINITELY);
return trigger;
}
@Bean
public CronTriggerFactoryBean trigger2(JobDetail job) {
CronTriggerFactoryBean trigger = new CronTriggerFactoryBean();
trigger.setJobDetail(job);
trigger.setCronExpression("0/20 * * ? * *");
return trigger;
}
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.
At the moment there's no support for multiple triggers in Sentry. I opted for a more consistent approach by getting the list of triggers so we don't update the monitor config on every invocation.
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.
We no longer send the schedule and also get the slug from the job data map. We'll document how to handle multiple triggers.
📜 Description
Add a
JobListener
for automatically reporting check-ins for scheduled Quartz jobs.💡 Motivation and Context
Quartz support for #2875
💚 How did you test it?
Manually with samples
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps