Skip to content

Automatic tracing for Jobs defined in quartz-jobs dependency#1170

Merged
eyalkoren merged 2 commits intoelastic:masterfrom
theobisproject:quartz-jobs-tracing
May 7, 2020
Merged

Automatic tracing for Jobs defined in quartz-jobs dependency#1170
eyalkoren merged 2 commits intoelastic:masterfrom
theobisproject:quartz-jobs-tracing

Conversation

@theobisproject
Copy link
Contributor

@theobisproject theobisproject commented May 1, 2020

What does this PR do?

While going through our traced Quartz schedules I noticed that the directory scanning schedules are missing which we implement via quartz-jobs artifact of the quartz project.
In this use case we do not implement a quartz job but only a callback interface the job will call. As described in the documentation I would have to add the org.quartz.jobs package currently to the application_packages to make this work.

In my opinion it would be more user friendly when the jobs provided by the quartz project would work without extra configuration.

This PR add's the instrumentation of the job from the quartz-jobs dependency. I decided to not add the package name to the applicationPackages collection because the collection we get is immutable.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Author's Checklist

Related issues

Use cases

The quartz-jobs dependency provides ready to use jobs for the following use-cases

  • Scanning directories for changed files via lastModified date
  • Scanning files for changes via lastModified date
  • Executing native executables
  • Sending mails
  • Invoke JMX
  • Sending messages to JMS
  • Invoke EJB

Screenshots

@ghost
Copy link

ghost commented May 1, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview stats

Expand to view the summary

Build stats

Test stats 🧪

Test Results
Failed 0
Passed 1243
Skipped 12
Total 1255

@codecov-io
Copy link

codecov-io commented May 3, 2020

Codecov Report

Merging #1170 into master will increase coverage by 4.47%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1170      +/-   ##
============================================
+ Coverage     59.66%   64.13%   +4.47%     
- Complexity       83     2792    +2709     
============================================
  Files           328      282      -46     
  Lines         14996    13586    -1410     
  Branches       2086     1871     -215     
============================================
- Hits           8948     8714     -234     
+ Misses         5438     4283    -1155     
+ Partials        610      589      -21     
Impacted Files Coverage Δ Complexity Δ
.../quartz/job/JobTransactionNameInstrumentation.java 100.00% <100.00%> (ø) 5.00 <0.00> (+5.00)
.../helper/KafkaInstrumentationHeadersHelperImpl.java
...acing/impl/ExternalSpanContextInstrumentation.java
...ent/kafka/KafkaProducerHeadersInstrumentation.java
...stic/apm/agent/kafka/BaseKafkaInstrumentation.java
...h/src/main/java/co/elastic/apm/attach/JvmInfo.java
.../elastic/apm/attach/ElasticAttachmentProvider.java
...m/agent/kafka/BaseKafkaHeadersInstrumentation.java
...ent/kafka/KafkaConsumerRecordsInstrumentation.java
...ent/opentracing/impl/OpenTracingTextMapBridge.java
... and 43 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec2b16c...683bde1. Read the comment docs.

@eyalkoren
Copy link
Contributor

Looks good! Thanks @theobisproject!
Please update the CHANGELOG and the supported technologies with a minimal sentence (maybe updating the existing NOTE: only classes from packages configured in application_packages will be instrumented.)

@theobisproject
Copy link
Contributor Author

I will update both. Should the sentence for the CHANGELOG be in the feature section?

@theobisproject theobisproject force-pushed the quartz-jobs-tracing branch from 965f78a to 683bde1 Compare May 6, 2020 17:01
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@eyalkoren eyalkoren requested a review from felixbarny May 7, 2020 05:25
@eyalkoren eyalkoren merged commit 73962e6 into elastic:master May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants