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

Excluding Spring-JMS specific Runnable from the concurrent plugin #1496

Merged
merged 4 commits into from
Nov 10, 2020

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Nov 10, 2020

What does this PR do?

Yet another stab on fixing the issue reported in the forum, without losing context propagation for JMS-poll-transactions altogether and with minimal effect on anything else.
Supersedes #1483.

Checklist

  • This is a bugfix
    • I have updated CHANGELOG.asciidoc
    • I have adjusted the current Spring JMS test so that it would fail without this fix

@codecov-io
Copy link

Codecov Report

Merging #1496 (7b3cc41) into master (feebf1a) will increase coverage by 9.70%.
The diff coverage is 76.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1496      +/-   ##
============================================
+ Coverage     59.31%   69.02%   +9.70%     
- Complexity       92     2625    +2533     
============================================
  Files           392      268     -124     
  Lines         17632    11721    -5911     
  Branches       2445     1542     -903     
============================================
- Hits          10458     8090    -2368     
+ Misses         6458     3112    -3346     
+ Partials        716      519     -197     
Impacted Files Coverage Δ Complexity Δ
...astic/apm/agent/impl/transaction/AbstractSpan.java 74.11% <0.00%> (-0.44%) 54.00 <0.00> (+54.00) ⬇️
...o/elastic/apm/agent/concurrent/JavaConcurrent.java 78.21% <81.81%> (+2.94%) 28.00 <11.00> (+28.00)
...m/agent/jms/JmsMessageListenerInstrumentation.java 17.50% <100.00%> (-5.76%) 5.00 <1.00> (+5.00) ⬇️
.../agent/dubbo/advice/ApacheMonitorFilterAdvice.java
.../elastic/apm/attach/ElasticAttachmentProvider.java
.../micrometer/MicrometerMeterRegistrySerializer.java
...duled/ScheduledTransactionNameInstrumentation.java
...m/agent/hibernatesearch/HibernateSearchHelper.java
...a/co/elastic/apm/agent/log/shipper/FileTailer.java
...ava/co/elastic/apm/opentracing/ApmSpanBuilder.java
... and 121 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 feebf1a...7b3cc41. Read the comment docs.

@apmmachine
Copy link
Contributor

apmmachine commented Nov 10, 2020

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1496 updated]

  • Start Time: 2020-11-10T15:32:54.535+0000

  • Duration: 46 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 1617
Skipped 12
Total 1629

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.

The overall approach LGTM, but tests are failing. Is it related to this PR?

@eyalkoren
Copy link
Contributor Author

tests are failing. Is it related to this PR?

It must be, but probably related to the concurrency plugin dependency. Checking...

@eyalkoren
Copy link
Contributor Author

Apparently what happened is that since a transaction is created and activated before each test starts, then when registering a MessageListener, this is done on a task that runs on a different thread and due to the concurrency plugin, the transaction is activated on this thread. Then, when onMessge is invoked, there is an active transaction on that thread, so the receive transaction is not created.

I split the Spring test into a separate module that has the dependencies on Spring JMS and on the concurrency plugin.

@eyalkoren
Copy link
Contributor Author

And now the pom thing 🤦
Fixing...

@eyalkoren eyalkoren merged commit da06dd5 into elastic:master Nov 10, 2020
@eyalkoren eyalkoren deleted the Spring-JMS-new-fix branch November 10, 2020 16:26
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