Skip to content
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

First "MVP" support for Spring @Scheduled / EJB @Schedule annotation #569

Merged
merged 4 commits into from
Apr 11, 2019

Conversation

timmhirsens
Copy link
Contributor

@timmhirsens timmhirsens commented Apr 9, 2019

This is a very basic implementation of #418

This has been tested in a unit test and with a local application against an elastic cloud cluster.

I am happy to iterate on this further.

This is a very basic implementation of elastic#418
if (active == null) {
transaction = tracer.startTransaction(TraceContext.asRoot(), null, clazz.getClassLoader())
.withName(signature)
.withType("scheduled")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a good choice for "type"? I did not find anything besided "request" in the current code base.

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

A few minor comments.

@timmhirsens timmhirsens changed the title First "MVP" support for Springs @Scheduled annotation First "MVP" support for Spring @Scheduled / EJB @Schedule annotation Apr 9, 2019
@felixbarny felixbarny requested a review from eyalkoren April 9, 2019 14:08
@codecov-io
Copy link

codecov-io commented Apr 10, 2019

Codecov Report

Merging #569 into master will decrease coverage by 2.53%.
The diff coverage is 66.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #569      +/-   ##
============================================
- Coverage     64.63%   62.09%   -2.54%     
  Complexity       68       68              
============================================
  Files           187      186       -1     
  Lines          7524     7029     -495     
  Branches        927      812     -115     
============================================
- Hits           4863     4365     -498     
+ Misses         2402     2388      -14     
- Partials        259      276      +17
Impacted Files Coverage Δ Complexity Δ
...astic/apm/agent/configuration/ServiceNameUtil.java 77.27% <0%> (-3.86%) 0 <0> (ø)
...lastic/apm/agent/impl/transaction/Transaction.java 71.42% <0%> (-0.88%) 0 <0> (ø)
...ic/apm/agent/servlet/ServletTransactionHelper.java 76.22% <85.71%> (-3.89%) 0 <0> (ø)
...apm/agent/bci/bytebuddy/CustomElementMatchers.java 75.67% <0%> (-13.34%) 0% <0%> (ø)
.../apm/agent/report/serialize/DslJsonSerializer.java 81.84% <0%> (-6.41%) 0% <0%> (ø)
...pm/agent/report/IntakeV2ReportingEventHandler.java 76.23% <0%> (-5.15%) 0% <0%> (ø)
...tic/apm/agent/configuration/CoreConfiguration.java 96.12% <0%> (-2.14%) 0% <0%> (ø)
...a/co/elastic/apm/agent/report/ReporterFactory.java 78.26% <0%> (-0.91%) 0% <0%> (ø)
... and 3 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 42c42d5...12413a7. Read the comment docs.

@felixbarny
Copy link
Member

I'm not sure why, but it seems to make the integration tests fail.

@timmhirsens
Copy link
Contributor Author

Hmm the TomcatIT failed first and now JBossIT failed.. Are they known to be flaky?

@felixbarny
Copy link
Member

Tests are green now, seems to be flakiness.

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.

Nice! ❤️

Can the container annotations Schedules come alone, or do they always contain several Schedule annotations? If they are only used as containers- can we overlook them?

Either way, if you can add a test for each of the Schedules annotation, that would be great.

Thanks!

@timmhirsens
Copy link
Contributor Author

I wrote test for the repeatable / container annotations and it turned out that we actually need to declare them in the method matcher.

@felixbarny
Copy link
Member

felixbarny commented Apr 11, 2019

That is because the compiler turns

@Scheduled(fixedDelay = 5)
@Scheduled(fixedDelay = 10)

into

@Schedules({
        @Scheduled(fixedDelay = 5),
        @Scheduled(fixedDelay = 10)
})

@eyalkoren eyalkoren merged commit 3fdca7d into elastic:master Apr 11, 2019
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