Add span links to messaging system tracing by eyalkoren · Pull Request #2610 · elastic/apm-agent-java

eyalkoren · 2022-05-03T12:09:14Z

What does this PR do?

Closes #2489:

Adding basic span links capabilities
Add span link to polling spans in all messaging plugins
Kafka: when there is an active span before the beginning of the iteration - add span links to it for each of the polled records. Otherwise- create a transaction per record as a child of the sending span.
SpringAmqpBatchMessageListenerInstrumentation - same as Kafka (if makes sense)
AWS Lambda functions triggered by SNS/SQS: create one transaction always and add span links
Adjust documentation
Tested end-to-end - see screenshots below
Add to CHANGELOG, also as potentially breaking change regarding SQS/SNS (losing message context data and not rendered in service maps)

ghost · 2022-05-03T12:44:43Z

💚 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: 2022-06-06T12:41:16.166+0000
Duration: 53 min 47 sec

Test stats 🧪

Test	Results
Failed	0
Passed	2939
Skipped	22
Total	2961

💚 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 tests.
run jdk compatibility tests : Run the JDK Compatibility tests.
run integration tests : Run the Agent Integration tests.
run end-to-end tests : Run the APM-ITs.
run windows tests : Build & tests on windows.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

eyalkoren · 2022-05-03T15:19:55Z

run integration tests

eyalkoren · 2022-05-04T14:22:04Z

run integration tests

AlexanderWert

LGTM

github-actions · 2022-05-30T13:57:41Z

/test

SylvainJuge

LGTM, mostly minor comments & questions.

SylvainJuge · 2022-05-31T09:24:45Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java

        errorPool.recycle(error);
    }

+    public void recycle(TraceContext traceContext) {


[minor] maybe rename this method to recycleSpanLink to avoid any confusion with recycling another TraceContext.

Suggested change

public void recycle(TraceContext traceContext) {

public void recycleSpanLink(TraceContext traceContext) {

Actually, it doesn't need to be specific for span links, it's for any potential use of pooled TraceContext, so I'll leave it just as is, similarly to all other recycle methods that differ by their argument type.

SylvainJuge · 2022-05-31T09:29:22Z

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java

+     */
+    public <H, C> boolean addSpanLink(TraceContext.ChildContextCreatorTwoArg<C, HeaderGetter<H, C>> childContextCreator,
+                                    HeaderGetter<H, C> headerGetter, @Nullable C carrier) {
+        if (spanLinks.size() == MAX_ALLOWED_SPAN_LINKS) {


[minor] we might reach the limit and still be fine if the span link is already in the spanLinks collection, we should probably issue this warning only when we are unable to add to the collection, which also means the limit has to be added to UniqueSpanLinkArrayList.

I started moving it, but losing the ability to log which span has the problem seems like a big compromise only for this extreme corner case where there are EXACTLY 1000 span links and repetitions in addition to that. Even if we encounter this somehow, this single logging is not so bad anyway.

apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/AbstractSpan.java

apm-agent-core/src/main/java/co/elastic/apm/agent/configuration/MessagingConfiguration.java

...kafka-base-plugin/src/main/java/co/elastic/apm/agent/kafka/KafkaConsumerInstrumentation.java

...headers-plugin/src/main/java/co/elastic/apm/agent/kafka/NewKafkaPollExitInstrumentation.java

...apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaClientVersionsIT.java

SylvainJuge · 2022-05-31T11:32:29Z

...-kafka-plugin/apm-kafka-headers-plugin/src/test/java/co/elastic/apm/agent/kafka/KafkaIT.java

+        assertThat(spans.size()).isGreaterThanOrEqualTo(1);
+        verifyPollSpanContents(spans);


[question] I'm not sure to understand here why we now have 1+ instead of exactly one previously.

You can't without looking at the other changes - see https://github.com/elastic/apm-agent-java/pull/2610/files#diff-12e2c27883cbfcbc62d039b9bf284b58b2a743637f207e7309a5f9ce101ba544R372-R388 - for the new test I had to change the polling method so that it polls until all records are consumed, so it is non-deterministic how many polls actually occur. The other alternative to make it robust is wait for quite long before polling, which I didn't want.
Since I verify that these spans have the expected contents, and they all combined contain the expected span links, I think this is fine and properly reflects real scenarios.

SylvainJuge · 2022-05-31T11:40:18Z

...c/main/java/co/elastic/apm/agent/rabbitmq/SpringAmqpBatchMessageListenerInstrumentation.java

+            List<Message> processedBatch = messageBatch;
+            Transaction batchTransaction = null;
+
+            if (tracer.isRunning() && messageBatch != null && !messageBatch.isEmpty()) {


[minor] maybe return early in case where tracer is not running or batch is empty to prevent some if nesting

SylvainJuge · 2022-05-31T11:41:27Z

...c/main/java/co/elastic/apm/agent/rabbitmq/SpringAmqpBatchMessageListenerInstrumentation.java

+                        active);
+                }
+
+                active = tracer.getActive();


Isn't the active variable already set with the active span/transaction on line 80 ?

Yes, but line 88 potentially activates a transaction. This basically says: "if there was an active span before, or if we just created one and activated it, I want to add span links in both cases".

Because the flow of this method is a bit complicated and because the return value is complex one that may be affected by multiple decision branches, I prefer one return in such methods (related to the comment above), but it's definitely a mater of taste

…ain/java/co/elastic/apm/agent/kafka/KafkaConsumerInstrumentation.java Co-authored-by: SylvainJuge <syl20j@gmail.com>

…c/main/java/co/elastic/apm/agent/kafka/NewKafkaPollExitInstrumentation.java Co-authored-by: SylvainJuge <syl20j@gmail.com>

eyalkoren · 2022-06-06T12:35:28Z

Below are screenshots reflecting the following scenario: a transaction sends two records to a Kafka "Request-Topic", waits for two corresponding reply records on a "Reply-Topic" and then iterates over the replies. Asynchronously, a "Batch-processing transaction" polls records from the "Request-Topic" and processes the record batch by sending a reply record for each request record.

The test transaction:

Each send span has a link to the batch-processing transaction:

Each poll span either contains no link if it returned without finding records or contains link/s to the corresponding span that reflects the send to the "Reply-Topic":

The transaction contains two links reflecting the two reply-records processing (linking to the corresponding send span):

On the batch processing side:

The transaction contains a link for each request record in the batch, corresponding the sending span:

Each reply record send span contains two links - one for the polling span and one for the processing span (the test transaction):

Add basic span links capabilities

a7c67f6

github-actions bot added the agent-java label May 3, 2022

Return whether a span link was added

0c9d906

Merge remote-tracking branch 'upstream/main' into span-links

e86fb17

eyalkoren changed the title ~~Add basic span links capabilities~~ Add span links to messaging system tracing May 4, 2022

eyalkoren mentioned this pull request May 4, 2022

Add support of kafka batch consumer #2601

Closed

eyalkoren added 5 commits May 4, 2022 11:20

Add to JMS polling spans

7cd46a8

Add to RabbitMQ polling spans

0ff1526

Add annoying Java 7 generics redundant type directives

e906a8d

Add to Kafka poll spans

70200ec

Upgrade tested Kafka broker and client to latest

d77e049

eyalkoren added 5 commits May 22, 2022 15:20

Merge remote-tracking branch 'upstream/main' into span-links

85a43b8

Add span links to Kafka parent span

800d692

Enforcing span links uniqueness

0738564

Add batch transaction with links to Spring AMQP

a97bbd6

Adjust Labmbda invocations triggered by SQS and SNS

3ae83c5

AlexanderWert reviewed May 30, 2022

View reviewed changes

eyalkoren marked this pull request as ready for review May 30, 2022 13:57

eyalkoren requested a review from SylvainJuge May 30, 2022 13:57

Add to docs and change batch strategy default

cf3698e

SylvainJuge approved these changes May 31, 2022

View reviewed changes

eyalkoren and others added 4 commits May 31, 2022 16:26

Update apm-agent-plugins/apm-kafka-plugin/apm-kafka-base-plugin/src/m…

b9874cf

…ain/java/co/elastic/apm/agent/kafka/KafkaConsumerInstrumentation.java Co-authored-by: SylvainJuge <syl20j@gmail.com>

Update apm-agent-plugins/apm-kafka-plugin/apm-kafka-headers-plugin/sr…

2f2baa7

…c/main/java/co/elastic/apm/agent/kafka/NewKafkaPollExitInstrumentation.java Co-authored-by: SylvainJuge <syl20j@gmail.com>

Merge remote-tracking branch 'upstream/main' into span-links

10b6440

Merge remote-tracking branch 'upstream/main' into span-links

ac680fa

eyalkoren added 3 commits June 6, 2022 15:37

Adding e2e disabled Kafka test

be77ed9

Merge remote-tracking branch 'upstream/main' into span-links

34d7fce

Adding to CHANGELOG

c21b8fe

eyalkoren enabled auto-merge (squash) June 6, 2022 12:41

eyalkoren merged commit 37e66e0 into elastic:main Jun 6, 2022

	public void recycle(TraceContext traceContext) {
	public void recycleSpanLink(TraceContext traceContext) {

		assertThat(spans.size()).isGreaterThanOrEqualTo(1);
		verifyPollSpanContents(spans);

Conversation

eyalkoren commented May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Uh oh!

ghost commented May 3, 2022 • 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

Uh oh!

eyalkoren commented May 3, 2022

Uh oh!

eyalkoren commented May 4, 2022

Uh oh!

AlexanderWert left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 30, 2022

Uh oh!

SylvainJuge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalkoren commented Jun 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eyalkoren commented May 3, 2022 •

edited

Loading

ghost commented May 3, 2022 •

edited by ghost

Loading

eyalkoren May 31, 2022 •

edited

Loading