Add instrumentation for Quartz 2.0 #4017
Conversation
anuraaga
requested review from
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
anuraaga
force-pushed
the
quartz-instrumentation
branch
from
6853697 to
8f7ca03
Compare
| /** | ||
| * Configures the {@link Scheduler} to enable tracing of jobs. | ||
| * | ||
| * <p><strong>NOTE:</strong> If there are job listeners already registered on the Scheduler that |
There was a problem hiding this comment.
Note that unlike something like AWS SDK instrumentation, where a bad executioninterceptor doesn't kill the entire SDK experience, a job listener that throws in the begin listener will cause the job itself to not run. Presumably quartz users or implementations of quartz listeners do know that they cannot throw exceptions or have the scheduler completely broken, not just tracing.
|
Build failure is OpenJ9 flake, will wait for comments / potential commits instead of rerunning |
|
|
||
| @Override | ||
| public void configure(Config config, IgnoredTypesBuilder builder) { | ||
| // Quartz executes jobs themselves in a synchronous, there's no reason to propagate context |
There was a problem hiding this comment.
"In a synchronous"? Do you mean that jobs themselves are executed synchronously?
|
|
||
| @Override | ||
| public void jobExecutionVetoed(JobExecutionContext jobExecutionContext) { | ||
| // TODO(anuraaga): Consider adding an attribute for vetoed jobs. |
There was a problem hiding this comment.
Is a veto similar to cancelling? We have some <someting>.cancelled attributes being added in various instrumentations.
There was a problem hiding this comment.
Let me leave this as a todo - I think cancelled is about right but lacking a semantic convention and significant knowledge of quartz will stick with simple for now.
| Throwable userError = error; | ||
| while (userError instanceof SchedulerException) { | ||
| userError = ((SchedulerException) userError).getUnderlyingException(); | ||
| } |
There was a problem hiding this comment.
Should we use an ErrorCauseExtractor for that?
| public class QuartzInstrumentationModule extends InstrumentationModule { | ||
|
|
||
| public QuartzInstrumentationModule() { | ||
| super("quartz", "quartz-1.7"); |
There was a problem hiding this comment.
Shouldn't it be 2.0? Same in the package name
| super("quartz", "quartz-1.7"); | |
| super("quartz", "quartz-2.0"); |
Quartz 2.0 was released 11 years ago so should be a reasonable target - it has significant API changes from 1.x that affect instrumentation, notably getting the job name from a
JobDetail.Fixes #3967