Skip to content

Conversation

@videnkz
Copy link
Contributor

@videnkz videnkz commented Feb 11, 2021

closes #1635

What does this PR do?

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • 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
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else
@videnkz videnkz marked this pull request as draft February 11, 2021 10:31
@ghost
Copy link

ghost commented Feb 11, 2021

💚 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 #1657 updated

  • Start Time: 2021-03-22T07:33:39.477+0000

  • Duration: 52 min 29 sec

  • Commit: 566ef69

Test stats 🧪

Test Results
Failed 0
Passed 1900
Skipped 15
Total 1915

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 1900
Skipped 15
Total 1915

@codecov-io
Copy link

codecov-io commented Feb 15, 2021

Codecov Report

Merging #1657 (2405042) into master (f16425b) will increase coverage by 4.60%.
The diff coverage is 40.54%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #1657 +/- ## ============================================ + Coverage 58.37% 62.98% +4.60%  - Complexity 92 3457 +3365  ============================================ Files 422 370 -52 Lines 18832 17009 -1823 Branches 2609 2339 -270 ============================================ - Hits 10994 10713 -281  + Misses 7064 5548 -1516  + Partials 774 748 -26 
Impacted Files Coverage Δ Complexity Δ
.../apm/agent/configuration/MetricsConfiguration.java 88.88% <ø> (ø) 1.00 <0.00> (+1.00)
...ic/apm/agent/rabbitmq/ConsumerInstrumentation.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...pm/agent/rabbitmq/AbstractBaseInstrumentation.java 9.37% <7.14%> (ø) 2.00 <1.00> (?)
...tic/apm/agent/rabbitmq/ChannelInstrumentation.java 18.47% <33.33%> (ø) 4.00 <1.00> (+4.00)
...gent/rabbitmq/header/AbstractTextHeaderGetter.java 83.33% <83.33%> (ø) 5.00 <5.00> (?)
...pm/agent/rabbitmq/RabbitmqBaseInstrumentation.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...gent/rabbitmq/header/RabbitMQTextHeaderGetter.java 100.00% <100.00%> (+16.66%) 2.00 <1.00> (+2.00)
...ent/kafka/KafkaConsumerRecordsInstrumentation.java
...va/co/elastic/apm/opentracing/ApmScopeManager.java
... and 60 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 f16425b...2405042. Read the comment docs.

@videnkz videnkz force-pushed the feature-rabbitmq-spring-amqp-simplemessagelistenercontainer branch from 6c12bc1 to 8c640fa Compare February 18, 2021 07:42
@videnkz videnkz marked this pull request as ready for review February 18, 2021 09:18
@videnkz
Copy link
Contributor Author

videnkz commented Feb 18, 2021

Hi @eyalkoren , @felixbarny
Please review this PR

@videnkz
Copy link
Contributor Author

videnkz commented Feb 18, 2021

image

image

@videnkz videnkz marked this pull request as draft February 18, 2021 10:18
@videnkz videnkz marked this pull request as ready for review February 18, 2021 13:28
@AlexanderWert AlexanderWert added this to the 7.13 milestone Feb 22, 2021
@eyalkoren
Copy link
Contributor

@kananindzya I finally got to this PR. You did great job! 👏

I started by just rearranging modules - since you already added a module for the tests, I moved all instrumentation code to it, leaving the base RabbitMQ module nondependent in Spring in any way.
I'll submit a review shortly.

Thanks for your invaluable contribution!! 🙏

@eyalkoren
Copy link
Contributor

@kananindzya well done!!
I did some additional minor changes and this part is ready.

What about supporting the batch API - org.springframework.amqp.core.MessageListener#onMessageBatch? I assume it will require similar approach to what we did with Kafka - starting/activating/deactivating/ending transactions during the iteration over the message batch. See https://github.com/elastic/apm-agent-java/blob/master/apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/src/main/java/co/elastic/apm/agent/kafka/helper/ConsumerRecordsIteratorWrapper.java.
It may require some effort, so makes sense to handle within a separate PR. WDYT?

@eyalkoren eyalkoren mentioned this pull request Mar 18, 2021
19 tasks
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.

As per the spec, the default exchange in RabbitMQ is represented by an empty string, which we want to replace with <default>. The Spring AMQP module should make use of the co.elastic.apm.agent.rabbitmq.AbstractBaseInstrumentation#normalizeExchangeName method for that.
@kananindzya, if you have a test case that uses the default exchange, please add it and I will add the required change.

@videnkz
Copy link
Contributor Author

videnkz commented Mar 18, 2021

As per the spec, the default exchange in RabbitMQ is represented by an empty string, which we want to replace with <default>. The Spring AMQP module should make use of the co.elastic.apm.agent.rabbitmq.AbstractBaseInstrumentation#normalizeExchangeName method for that.
@kananindzya, if you have a test case that uses the default exchange, please add it and I will add the required change.

please check new test

@eyalkoren eyalkoren merged commit 789406b into elastic:master Mar 22, 2021
@hectorespert hectorespert mentioned this pull request Apr 24, 2021
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants