Add APIs for manual inter process context propagation #396

felixbarny · 2018-12-21T09:31:40Z

closes #334

felixbarny · 2018-12-21T09:35:57Z

This is an alternative design to #394

It has a much nicer API geared towards Java 8 but also provides fallbacks for Java 7.

For this to work on Java 7 we rely on the ability of a JVM to lazily link the method parameters so there is a small risk that on certain JVM implementations, the API does not work anymore for Java 7.

closes elastic#334

felixbarny · 2018-12-21T09:52:17Z

@kuisathaverat Pipeline Jenkins does not like that PR

felixbarny · 2018-12-21T10:13:10Z

TLDR;

adds these methods:

ElasticApm

Transaction startTransactionWithRemoteParent(HeaderExtractor)
Transaction startTransactionWithRemoteParent(HeaderExtractor, HeadersExtractor)

Span

void injectTraceHeaders(HeaderInjector)

eyalkoren

I don't think instance methods are guaranteed not to be eagerly linked by all JVM implementations. This is what I found in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-5.html#jvms-5.4:

a Java Virtual Machine implementation may choose to resolve each symbolic reference in a class or interface individually when it is used ("lazy" or "late" resolution), or to resolve them all at once when the class is being verified ("eager" or "static" resolution).

Maybe we can only rely on the class-separation you did with ElasticApm - if we only want to inject context of active span we can use Tracer static method. Do you think making that a requirement is limiting? If we do that it would be safer, cleaner and even more similar to OpenTracing.

An even cleaner API would be usage of our interfaces instead of the Java 8 ones, and invoke the provided implementations through reflection in the actual tracer code, so to avoid dependency. You could even cache method handles based on implementation class name to significantly improve performance. Not as performant as method references as it also requires instance allocations, but much cleaner.

Other than that, maybe find a better word instead of stuck for Java 7 users:)

eyalkoren · 2018-12-24T14:43:15Z

apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java

+ *
+ * When stuck on Java 7, just call ElasticApmJava7.startTransaction().
+ * Observation: actually, it also seems to work to call ElasticApm.startTransaction().
+ * I assume the JVM does not eagerly link the methods when only referring to static methods of the super class.


I wouldn't rely on anything based on testing in this regard, only on the spec for everything that is JVM implementation dependent. I'd say Java 7 users should only use ElasticApmJava7.

eyalkoren · 2018-12-25T07:29:48Z

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

+    private static final ChildContextCreator<Map<String, ? extends Iterable<String>>> FROM_MAP = new ChildContextCreator<Map<String, ? extends Iterable<String>>>() {
+        @Override
+        public boolean asChildOf(TraceContext child, Map<String, ? extends Iterable<String>> headers) {
+            final Iterator<String> iterator = headers.get(TRACE_PARENT_HEADER).iterator();


headers.get(TRACE_PARENT_HEADER) can produce null - better to log that instead of throwing NPE

...m-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java

eyalkoren · 2018-12-25T08:00:27Z

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

@@ -160,6 +178,25 @@ public static TraceContext with128BitId() {
        return AS_ROOT;
    }

+    @IgnoreJRERequirement
+    public static ChildContextCreator<Function<String, String>> fromHeaders() {


Risky in Java 7

codecov-io · 2018-12-25T08:20:03Z

Codecov Report

Merging #396 into master will decrease coverage by 0.18%.
The diff coverage is 52.5%.

@@             Coverage Diff              @@
##             master     #396      +/-   ##
============================================
- Coverage     72.26%   72.08%   -0.19%     
+ Complexity     1339     1314      -25     
============================================
  Files           145      144       -1     
  Lines          5257     5086     -171     
  Branches        549      523      -26     
============================================
- Hits           3799     3666     -133     
+ Misses         1222     1182      -40     
- Partials        236      238       +2

Impacted Files	Coverage Δ	Complexity Δ
...astic/apm/agent/impl/transaction/TraceContext.java	`91.66% <0%> (-1.07%)`	`56 <0> (-11)`
...bci/bytebuddy/FailSafeDeclaredMethodsCompiler.java	`100% <100%> (ø)`	`4 <4> (ø)`	⬇️
...agent/plugin/api/ElasticApmApiInstrumentation.java	`37.5% <13.33%> (-2.5%)`	`3 <0> (ø)`
.../agent/plugin/api/AbstractSpanInstrumentation.java	`51.92% <40%> (-0.12%)`	`3 <0> (-3)`
...java/co/elastic/apm/agent/bci/ElasticApmAgent.java	`70.13% <60%> (-3.24%)`	`21 <0> (-3)`
.../elastic/apm/agent/bci/bytebuddy/MatcherTimer.java	`62.5% <0%> (-31.25%)`	`4% <0%> (-3%)`
...c/apm/agent/bci/MatcherTimerLifecycleListener.java	`75% <0%> (-18.75%)`	`4% <0%> (-1%)`
... and 2 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 f1611f9...c34900f. Read the comment docs.

Those can also be implemented via a lambda expression but don't have a Java 7/8 compatibility issue. The interfaces are invoked via a MethodHandle which is constructed in the API and read from method arguments in the instrumentation. Therefore, the instrumentation does not have to dynamically create MethodHandle or Method objects.

felixbarny · 2018-12-26T09:33:00Z

...t-core/src/main/java/co/elastic/apm/agent/bci/bytebuddy/FailSafeDeclaredMethodsCompiler.java

+ * on exceptions in that it ignores methods causing exceptions.
+ * </p>
+ */
+public enum FailSafeDeclaredMethodsCompiler implements MethodGraph.Compiler {


not actually needed for this change anymore but might still be relevant

eyalkoren

Excellent!!
Few comments and some leftovers from the former version.

...m-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java

eyalkoren · 2018-12-26T16:47:24Z

...t-core/src/main/java/co/elastic/apm/agent/bci/bytebuddy/FailSafeDeclaredMethodsCompiler.java

+ * we eagerly resolve the parameters of all methods and exclude problematic methods.
+ * </p>
+ * <p>
+ * One use case of this is to be able to instrument classes implementing {@code co.elastic.apm.api.Span},


Not a relevant use case anymore, but the use case I ran into of failing to instrument due to lookup restrictions in bootstrap CL is one, and the more common (I assume) is class that is loaded (and not yet linked) when its dependencies are not available in the classpath yet.

...m-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java

...pm-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/AbstractSpanInstrumentation.java

...ugin/src/main/resources/META-INF/services/co.elastic.apm.agent.bci.ElasticApmInstrumentation

...m-api-plugin/src/main/java/co/elastic/apm/agent/plugin/api/ElasticApmApiInstrumentation.java

...lugins/apm-api-plugin/src/test/java/co/elastic/apm/api/ElasticApmApiInstrumentationTest.java

eyalkoren · 2018-12-26T17:25:25Z

apm-agent-api/src/main/java/co/elastic/apm/api/ElasticApm.java

+     * @since 1.3.0
+     */
+    @Nonnull
+    public static Transaction startTransactionWithRemoteParent(HeaderExtractor headerExtractor, HeadersExtractor headersExtractor) {


I think either replace this one to only accept HeadersExtractor or add a third API method that accepts only that and uses the same private static method with null as the first argument

even for protocols which do support multiple headers, we want to have the ability to get a single value for a header in case we know it can only have a single value, like traceparent.

…xt-propagation

felixbarny requested a review from eyalkoren December 21, 2018 09:36

Add APIs for manual inter process context propagation

460d71a

closes elastic#334

felixbarny force-pushed the inter-process-context-propagation branch from 59a45a5 to 460d71a Compare December 21, 2018 09:49

felixbarny added the [zube]: In Review label Dec 21, 2018

felixbarny self-assigned this Dec 21, 2018

eyalkoren reviewed Dec 25, 2018

View reviewed changes

felixbarny commented Dec 26, 2018

View reviewed changes

Remove unnedded ChildContextCreators

c28ed56

eyalkoren reviewed Dec 26, 2018

View reviewed changes

felixbarny mentioned this pull request Jan 7, 2019

Add ability to get tracing headers from API #394

Closed

felixbarny and others added 5 commits January 7, 2019 12:40

Review comments

1c0d845

Merge remote-tracking branch 'origin/master' into inter-process-conte…

1d4c803

…xt-propagation

Implement propagation with HeadersExtractor

b398b07

Add changelog

08756bd

Merge branch 'master' into inter-process-context-propagation

c34900f

eyalkoren approved these changes Jan 7, 2019

View reviewed changes

felixbarny merged commit e2261a9 into elastic:master Jan 7, 2019

felixbarny deleted the inter-process-context-propagation branch January 7, 2019 16:10

zube bot added [zube]: Done and removed [zube]: In Review labels Jan 7, 2019

alvarolobato removed the [zube]: Done label Jan 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APIs for manual inter process context propagation #396

Add APIs for manual inter process context propagation #396

felixbarny commented Dec 21, 2018 •

edited

Loading

felixbarny commented Dec 21, 2018

felixbarny commented Dec 21, 2018

felixbarny commented Dec 21, 2018 •

edited

Loading

eyalkoren left a comment

eyalkoren Dec 24, 2018

eyalkoren Dec 25, 2018

eyalkoren Dec 25, 2018

codecov-io commented Dec 25, 2018 •

edited

Loading

felixbarny Dec 26, 2018 •

edited

Loading

eyalkoren left a comment

eyalkoren Dec 26, 2018

eyalkoren Dec 26, 2018

felixbarny Jan 7, 2019

Add APIs for manual inter process context propagation #396

Add APIs for manual inter process context propagation #396

Conversation

felixbarny commented Dec 21, 2018 • edited Loading

felixbarny commented Dec 21, 2018

felixbarny commented Dec 21, 2018

felixbarny commented Dec 21, 2018 • edited Loading

eyalkoren left a comment

Choose a reason for hiding this comment

eyalkoren Dec 24, 2018

Choose a reason for hiding this comment

eyalkoren Dec 25, 2018

Choose a reason for hiding this comment

eyalkoren Dec 25, 2018

Choose a reason for hiding this comment

codecov-io commented Dec 25, 2018 • edited Loading

Codecov Report

felixbarny Dec 26, 2018 • edited Loading

Choose a reason for hiding this comment

eyalkoren left a comment

Choose a reason for hiding this comment

eyalkoren Dec 26, 2018

Choose a reason for hiding this comment

eyalkoren Dec 26, 2018

Choose a reason for hiding this comment

felixbarny Jan 7, 2019

Choose a reason for hiding this comment

felixbarny commented Dec 21, 2018 •

edited

Loading

felixbarny commented Dec 21, 2018 •

edited

Loading

codecov-io commented Dec 25, 2018 •

edited

Loading

felixbarny Dec 26, 2018 •

edited

Loading