added support to spring amqp batch by videnkz · Pull Request #1716 · elastic/apm-agent-java

videnkz · 2021-03-26T09:43:17Z

closes #1706

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

ghost · 2021-03-26T09:46:44Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2021-11-02T05:36:41.598+0000
Duration: 73 min 14 sec
Commit: 1fcac13

Test stats 🧪

Test	Results
Failed	0
Passed	2492
Skipped	19
Total	2511

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.
run benchmark tests : Run the benchmark test.
run compatibility tests : Run the JDK Compatibility test.
run integration tests : Run the APM-ITs.

codecov-io · 2021-03-30T12:16:10Z

Codecov Report

Merging #1716 (918f877) into master (1745351) will decrease coverage by 8.11%.
The diff coverage is 33.33%.

❗ Current head 918f877 differs from pull request most recent head 76f3336. Consider uploading reports for the commit 76f3336 to get more accurate results

@@             Coverage Diff              @@
##             master    #1716      +/-   ##
============================================
- Coverage     69.42%   61.30%   -8.12%     
- Complexity     2789     3621     +832     
============================================
  Files           291      401     +110     
  Lines         12413    18249    +5836     
  Branches       1616     2507     +891     
============================================
+ Hits           8618    11188    +2570     
- Misses         3214     6278    +3064     
- Partials        581      783     +202

Impacted Files	Coverage Δ	Complexity Δ
...apm/agent/log/shader/AbstractLogShadingHelper.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...astic/apm/agent/log4j1/Log4j1LogShadingHelper.java	`22.22% <0.00%> (-2.78%)`	`4.00 <0.00> (ø)`
...astic/apm/agent/log4j2/Log4j2LogShadingHelper.java	`11.76% <0.00%> (-2.53%)`	`3.00 <0.00> (ø)`
...stic/apm/agent/jms/JmsMessagePropertyAccessor.java	`79.16% <50.00%> (ø)`	`7.00 <0.00> (?)`
...o/elastic/apm/agent/mdc/MdcActivationListener.java	`82.27% <100.00%> (ø)`	`16.00 <0.00> (?)`
.../apm/agent/servlet/FilterChainInstrumentation.java	`100.00% <0.00%> (ø)`	`5.00% <0.00%> (?%)`
...itmq/SpringAmqpMessageListenerInstrumentation.java	`14.28% <0.00%> (ø)`	`4.00% <0.00%> (?%)`
...ava/co/elastic/apm/opentracing/ElasticApmTags.java	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (?%)`
.../elastic/apm/agent/servlet/ServletGlobalState.java	`75.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
...t/urlconnection/UrlConnectionPropertyAccessor.java	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (?%)`
... and 109 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 1745351...76f3336. Read the comment docs.

eyalkoren

Nice! 👏
A few minor comments, overall looks really good!
I am pushing a small fix that will allow testing.

eyalkoren · 2021-06-06T13:30:36Z

...rabbitmq-spring/src/main/java/co/elastic/apm/agent/rabbitmq/MessageBatchIteratorWrapper.java

+            if (message == null) {
+                return message;
+            }


message shouldn't be null.
In such case, next() would throw NoSuchElementException, so you can just remove this check and rely on the original implementation.

eyalkoren · 2021-06-06T13:40:44Z

...itmq/apm-rabbitmq-spring/src/main/java/co/elastic/apm/agent/rabbitmq/MessageBatchHelper.java

+public interface MessageBatchHelper<MSG> {
+
+    List<MSG> wrapMessageBatchList(List<MSG> messageBatchList);
+}


There is no need for a generic interface, as long as the helpers are only being referenced by advices.
The advice classes can depend on helpers with specific AMQP dependencies (as opposed to the instrumentation classes)

eyalkoren · 2021-06-06T13:43:11Z

...m-rabbitmq-spring/src/main/java/co/elastic/apm/agent/rabbitmq/SpringBaseInstrumentation.java

    }
+
+    static class BaseAdvice {
+        protected static final MessageBatchHelper messageBatchHelper;


Shouldn't this be on the batch advice instead of the base advice?

moved to SpringAmqpBatchMessageListenerInstrumentation$MessageListenerContainerWrappingAdvice

eyalkoren · 2021-06-06T13:53:30Z

...rabbitmq-spring/src/main/java/co/elastic/apm/agent/rabbitmq/SpringAmqpTransactionHelper.java

+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+public interface SpringAmqpTransactionHelper {


Why is this interface required? Can't we just use the single impl as is?

yes, this interface not required. Deleted

eyalkoren · 2021-06-06T14:11:33Z

...rabbitmq-spring/src/main/java/co/elastic/apm/agent/rabbitmq/MessageBatchIteratorWrapper.java

+            if (messageProperties == null) {
+                return message;
+            }


Why? We should create a transaction anyway - a root transaction if there is no traceparent property, or child transaction if there is. We already do that (create a transaction when there is no property). The same goes for SpringAmqpMessageListenerInstrumentation - we should allow null properties.
Please fix SpringAmqpTransactionHelper#createTransaction() instead to allow for null properties (and treat the same as if there are properties without context property).

…rt-spring-amqp-batch

…amqp-batch' into feature-add-support-spring-amqp-batch

eyalkoren · 2021-06-20T13:49:25Z

Thanks for applying the review comments ❤️

Please see my last commits - I only removed another interface, changed helper names, added to CHANGELOG and added a test for distributed tracing.

Please let me know if you disagree with any of this.

…rt-spring-amqp-batch

eyalkoren · 2021-11-01T17:37:25Z

run elasticsearch-ci/docs

eyalkoren · 2021-11-02T05:12:54Z

@kananindzya we removed our AssignTo annotations and migrated to use the new Byte Buddy AssignReturned annotations - see other instrumentations and please it can build and test pass.
Sorry about that 🙁

videnkz · 2021-11-02T05:36:40Z

@kananindzya we removed our AssignTo annotations and migrated to use the new Byte Buddy AssignReturned annotations - see other instrumentations and please it can build and test pass. Sorry about that slightly_frowning_face

fixed

eyalkoren · 2021-11-02T08:50:56Z

run elasticsearch-ci/docs

added support to spring amqp batch

d8935b8

github-actions bot added the agent-java label Mar 26, 2021

added transaction hepler for spring amqp

76f3336

Merge branch 'master' into feature-add-support-spring-amqp-batch

12a32fa

eyalkoren reviewed Jun 6, 2021

View reviewed changes

eyalkoren and others added 7 commits June 6, 2021 17:22

Adjusting getAdviceClass to new form

0d2a94e

fixes according to comments

70194a7

make messageProperties nullable

4d1356c

Merge remote-tracking branch 'upstream/master' into feature-add-suppo…

49b171c

…rt-spring-amqp-batch

Merge remote-tracking branch 'upstream/master' into feature-add-suppo…

e2a7803

…rt-spring-amqp-batch

Merge remote-tracking branch 'kananindzya/feature-add-support-spring-…

48fbec8

…amqp-batch' into feature-add-support-spring-amqp-batch

Minor refactor and adding test for distributed tracing

0ccf528

eyalkoren force-pushed the feature-add-support-spring-amqp-batch branch from 1eec8f3 to 0ccf528 Compare June 20, 2021 13:43

Adding to CHANGELOG

e99981b

eyalkoren approved these changes Jun 20, 2021

View reviewed changes

eyalkoren mentioned this pull request Jun 21, 2021

Adding special wrapper for ChannelAwareMessageListener #1872

Merged

3 tasks

videnkz added 4 commits June 21, 2021 12:05

Merge remote-tracking branch 'upstream/master' into feature-add-suppo…

bc03f24

…rt-spring-amqp-batch

merge conflict resolves

780fbda

Merge remote-tracking branch 'upstream/master' into feature-add-suppo…

1600f03

…rt-spring-amqp-batch

merge conflict resolved

4460994

SylvainJuge added the new-feature New feature label Aug 30, 2021

videnkz added 2 commits October 13, 2021 22:55

merge conflict resolved

97825c6

merge conflict resolved

1029956

eyalkoren enabled auto-merge (squash) November 1, 2021 17:40

fixed annotation

1fcac13

auto-merge was automatically disabled November 2, 2021 05:36
Head branch was pushed to by a user without write access

eyalkoren enabled auto-merge (squash) November 2, 2021 08:51

eyalkoren merged commit c0a4ee6 into elastic:master Nov 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support to spring amqp batch#1716

added support to spring amqp batch#1716
eyalkoren merged 18 commits intoelastic:masterfrom
videnkz:feature-add-support-spring-amqp-batch

videnkz commented Mar 26, 2021 •

edited by zube bot

Loading

ghost commented Mar 26, 2021 •

edited by ghost

Loading

Build stats

Test stats 🧪

codecov-io commented Mar 30, 2021

eyalkoren left a comment

eyalkoren Jun 6, 2021

videnkz Jun 9, 2021

eyalkoren Jun 6, 2021

videnkz Jun 9, 2021

eyalkoren Jun 6, 2021

videnkz Jun 9, 2021

eyalkoren Jun 6, 2021

videnkz Jun 9, 2021

eyalkoren Jun 6, 2021 •

edited

Loading

videnkz Jun 9, 2021

eyalkoren commented Jun 20, 2021

eyalkoren commented Nov 1, 2021

eyalkoren commented Nov 2, 2021

videnkz commented Nov 2, 2021

eyalkoren commented Nov 2, 2021

Labels

4 participants

Conversation

videnkz commented Mar 26, 2021 • edited by zube bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

ghost commented Mar 26, 2021 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

💚 Flaky test report

🤖 GitHub comments

codecov-io commented Mar 30, 2021

Codecov Report

eyalkoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalkoren Jun 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalkoren commented Jun 20, 2021

eyalkoren commented Nov 1, 2021

eyalkoren commented Nov 2, 2021

videnkz commented Nov 2, 2021

eyalkoren commented Nov 2, 2021

Labels

4 participants

videnkz commented Mar 26, 2021 •

edited by zube bot

Loading

ghost commented Mar 26, 2021 •

edited by ghost

Loading

eyalkoren Jun 6, 2021 •

edited

Loading