Context propagation refactorings #3206

JonasKunz · 2023-06-26T08:23:49Z

What does this PR do?

Move the following responsibilites from Span to ElasticContext:

Creation of child spans
Context propagation

In addition to make plugins simpler I decided to ensure that ElasticContext is never null by returning an empty context instead. This avoids a lot of null-checks in the plugins.

Part of #3004 . This PR also changes that context propagation always happens, even if there is no active span.
E.g. if someone just updated the baggage, we want to still create baggage headers, even if no trace-context will be propagated.

This PR is a refactoring and should NOT change any actual functionality of the agent.

Checklist

This is something else
- ~~I have updated CHANGELOG.asciidoc~~

Moved propagateContext from span to context

ghost · 2023-06-26T08:38:15Z

💚 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: 2023-07-03T11:39:14.976+0000
Duration: 14 min 53 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the 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!)

JonasKunz · 2023-06-26T10:10:34Z

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

            return ((ElasticContextWrapper<?>) current).getWrappedContext();
        }
-        return current;
+        return current != null ? current : EmptyElasticContext.INSTANCE;


This change has been to simplify plugins: Instead of returning null when there is no context, we return an empty context. This avoids a lot of null-checks in the plugins.
If one really needs to know wheter a context is active or not, it is possible to do so by calling ElasticContext.isEmpty(). This is for example useful to optimize intra-process context propagation: No need to propagate empty contexts here.

JonasKunz · 2023-06-26T10:12:23Z

apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/ElasticContext.java

+
+import javax.annotation.Nullable;
+
+public interface ElasticContext<T extends ElasticContext<T>> extends ReferenceCounted, Activateable<T> {


ElasticContext is now reference counted. The reason is that context can reference spans and spans are reference counted.
E.g. if you were to propagate a context to another thread, you need to make sure to do an incrementReferenceCount on the context, just like we did for Spans until now.

JonasKunz · 2023-06-26T10:13:28Z

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


-        AbstractSpan<?> span = context.getSpan();
-
-        if (activeContext != context && activeContext instanceof ElasticContextWrapper) {


This unwrapping was unnecessary: currentContext has already been doing the unwrapping.

github-actions · 2023-06-26T11:40:51Z

/test

SylvainJuge · 2023-06-26T11:45:36Z

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

            return false;
        }

+        context.incrementReferences();


here I am not sure why we increment references in the first place, but I think it's because the span reference is being used in the activation listeners, so might be worth checking if that does not mean systematically incrementing the context references count even when not needed.

We do increment here to make sure that the Span (and now context) is not recycled while it is on the ActiveStack.
The same is now required for contexts, because we need to ensure that the underlying span is kept alive.

SylvainJuge · 2023-06-26T12:06:07Z

...ent3-plugin/src/main/java/co/elastic/apm/agent/httpclient/v3/HttpClient3Instrumentation.java

-            } catch (Exception e) {
+            Span<?> span = null;
+            final ElasticContext<?> parent = TracerAwareInstrumentation.tracer.currentContext();
+            if (parent.getSpan() != null) {


[nitpicking] keeping the shortcut return when there is no span avoids one level of if nesting.

Sorry, should have mentioned it in the PR description (updated it now):
This PR also changes that context propagation always happens, even if there is no active span.
E.g. if someone just updated the baggage, we want to still create baggage headers, even if no trace-context will be propagated. Baggage is conceptually independent from tracing.

For this reason I had to remove the early exits in order to make sure that the context propagation part is always reached.

...n/src/main/java/co/elastic/apm/agent/httpclient/v4/ApacheHttpAsyncClientInstrumentation.java

...plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/ApacheHttpClientInstrumentation.java

apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/ElasticContext.java

.../src/main/java/co/elastic/apm/agent/httpclient/v4/LegacyApacheHttpClientInstrumentation.java

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

apm-agent-tracer/src/main/java/co/elastic/apm/agent/tracer/ElasticContext.java

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

SylvainJuge

LGTM, I'm mostly nit-picking about minor style things, congrats on this huge refactor !!

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

...ent3-plugin/src/main/java/co/elastic/apm/agent/httpclient/v3/HttpClient3Instrumentation.java

...n/src/main/java/co/elastic/apm/agent/httpclient/v4/ApacheHttpAsyncClientInstrumentation.java

...plugin/src/main/java/co/elastic/apm/agent/httpclient/v4/ApacheHttpClientInstrumentation.java

...c/test/java/co/elastic/apm/agent/httpclient/v4/ApacheHttpAsyncClientInstrumentationTest.java

...t-plugins/apm-struts-plugin/src/main/java/co/elastic/apm/agent/struts/ActionProxyAdvice.java

...lugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java

.../apm-vertx-common/src/main/java/co/elastic/apm/agent/vertx/AbstractVertxWebClientHelper.java

...plugin/src/main/java/co/elastic/apm/agent/vertx/v4/webclient/HttpContextInstrumentation.java

# Conflicts: # apm-agent-core/src/main/java/co/elastic/apm/agent/impl/ElasticApmTracer.java # apm-agent-core/src/main/java/co/elastic/apm/agent/tracemethods/TraceMethodInstrumentation.java # apm-agent-plugins/apm-apache-httpclient/apm-apache-httpclient3-plugin/src/main/java/co/elastic/apm/agent/httpclient/v3/HttpClient3Instrumentation.java # apm-agent-plugins/apm-asynchttpclient-plugin/src/main/java/co/elastic/apm/agent/asynchttpclient/AbstractAsyncHttpClientInstrumentation.java # apm-agent-plugins/apm-aws-sdk/apm-aws-sdk-common/src/main/java/co/elastic/apm/agent/awssdk/common/AbstractSQSInstrumentationHelper.java # apm-agent-plugins/apm-dubbo-plugin/src/main/java/co/elastic/apm/agent/dubbo/advice/ApacheMonitorFilterAdvice.java # apm-agent-plugins/apm-es-restclient-plugin/apm-es-restclient-plugin-common/src/main/java/co/elastic/apm/agent/esrestclient/ElasticsearchRestClientInstrumentationHelper.java # apm-agent-plugins/apm-finagle-httpclient-plugin/src/main/java/co/elastic/apm/agent/finaglehttpclient/FinaglePayloadSizeFilterInstrumentation.java # apm-agent-plugins/apm-javalin-plugin/src/main/java/co/elastic/apm/agent/javalin/JavalinInstrumentation.java # apm-agent-plugins/apm-javalin-plugin/src/main/java/co/elastic/apm/agent/javalin/JavalinRenderInstrumentation.java # apm-agent-plugins/apm-jdbc-plugin/src/main/java/co/elastic/apm/agent/jdbc/helper/JdbcHelper.java # apm-agent-plugins/apm-jms-plugin/apm-jms-plugin-base/src/main/java/co/elastic/apm/agent/jms/JmsInstrumentationHelper.java # apm-agent-plugins/apm-jms-plugin/apm-jms-plugin-base/src/main/java/co/elastic/apm/agent/jms/JmsMessageConsumerInstrumentation.java # apm-agent-plugins/apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletApiAdvice.java # apm-agent-plugins/apm-spring-resttemplate/apm-spring-resttemplate-plugin/src/main/java/co/elastic/apm/agent/resttemplate/SpringRestTemplateAdvice.java # apm-agent-plugins/apm-spring-webflux/apm-spring-webclient-plugin/src/main/java/co/elastic/apm/agent/springwebclient/WebClientExchangeFunctionInstrumentation.java # apm-agent-plugins/apm-spring-webmvc/apm-spring-webmvc-spring5/src/main/java/co/elastic/apm/agent/springwebmvc/ViewRenderInstrumentation.java # apm-agent-plugins/apm-urlconnection-plugin/src/main/java/co/elastic/apm/agent/urlconnection/HttpUrlConnectionInstrumentation.java

JonasKunz added 2 commits June 26, 2023 10:20

Moved propagateContext from span to context

58b022a

Moved propagateContext from span to context

Moved API capability for starting spans from spans to context

fd45dab

JonasKunz added the ci:agent-integration Enables agent integration tests in build pipeline label Jun 26, 2023

github-actions bot added the agent-java label Jun 26, 2023

JonasKunz mentioned this pull request Jun 26, 2023

[META 757] Baggage in APM Agents #3004

Closed

Fix javadoc, move shouldSkipChildSpanCreation check to context

71ca343

JonasKunz commented Jun 26, 2023

View reviewed changes

JonasKunz marked this pull request as ready for review June 26, 2023 11:40

JonasKunz requested a review from SylvainJuge June 26, 2023 11:40

SylvainJuge reviewed Jun 26, 2023

View reviewed changes

Review fixes

8f32172

SylvainJuge reviewed Jun 30, 2023

View reviewed changes

SylvainJuge approved these changes Jun 30, 2023

View reviewed changes

JonasKunz added 5 commits July 3, 2023 10:53

Revert getSpan() method renaming

78a2c8c

More review fixes

0d485db

Renamed parent to activeContext

d9e1fcd

Further review fixes

1845d12

JonasKunz enabled auto-merge (squash) July 10, 2023 12:33

JonasKunz merged commit 86184c5 into elastic:main Jul 10, 2023

JonasKunz deleted the baggage-propagation branch July 10, 2023 14:06

JonasKunz mentioned this pull request Jul 10, 2023

Fix empty context activation behaviour #3227

Merged

2 tasks

JonasKunz added a commit to JonasKunz/apm-agent-java that referenced this pull request Jul 25, 2023

Fix invalid SQS propagation introduced with elastic#3206

3e084ea

JonasKunz added a commit that referenced this pull request Jul 26, 2023

Fix invalid SQS propagation introduced with #3206 (#3254)

1b7e461


		import javax.annotation.Nullable;

		public interface ElasticContext<T extends ElasticContext<T>> extends ReferenceCounted, Activateable<T> {


		AbstractSpan<?> span = context.getSpan();

		if (activeContext != context && activeContext instanceof ElasticContextWrapper) {

Context propagation refactorings #3206

Context propagation refactorings #3206

Uh oh!

Conversation

JonasKunz commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Uh oh!

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

Uh oh!

💚 Build Succeeded

Build stats

❕ Flaky test report

🤖 GitHub comments

Uh oh!

JonasKunz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

JonasKunz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

JonasKunz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 26, 2023

Uh oh!

SylvainJuge Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

JonasKunz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

SylvainJuge Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

JonasKunz Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SylvainJuge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JonasKunz commented Jun 26, 2023 •

edited

Loading

ghost commented Jun 26, 2023 •

edited by ghost

Loading