Grizzly-http and grizzly-client instrumentation #1365

heathkd · 2020-04-13T16:03:24Z

The Mulesoft instrumentation traces API calls made from HttpListener and HttpRequester components. Under the hood, these components use grizzly for both the server and client logic. As a result, this PR adds instrumentation to Grizzly.

For development purposes, the instrumentation is currently located in the mule-http-connector module of the tracer. However, it should be added to the grizzly module before being merged.

heathkd · 2020-04-13T16:10:35Z

The instrumentation in the "clientV2" package is some experimental instrumentation where the client span is created from HttpClientFilter.java instead of AsyncHttpClient.java. However, I'd like to focus on using the instrumentation in the client packaged and the server package.

Currently, the client and server span are being sent as separate traces. I would like to consolidate the two spans into one trace. If there is anything obvious that is missing and should be added to ClientAdvice.java or ServerAdvice.java to make this happen, please let me know.

heathkd · 2020-04-20T14:19:05Z

...main/java/datadog/trace/instrumentation/java/concurrent/AbstractExecutorInstrumentation.java

+        "org.glassfish.grizzly.threadpool.AbstractThreadPool",
+        "org.glassfish.grizzly.threadpool.FixedThreadPool",
        "org.glassfish.grizzly.threadpool.GrizzlyExecutorService",
+        "org.glassfish.grizzly.threadpool.QueueLimitedThreadPool",
+        "org.glassfish.grizzly.threadpool.SyncThreadPool",


Executor instrumentation was skipped for these four executors. FixedThreadPool, specifically, is instantiated by GrizzlyExecutorService after HttpServerFilter receives a request. QueueLimitedThreadPool is created if a FixedThreadPool cannot be created. Fixed, QueueLimited, and Sync all extend AbstractThreadPool. Would it make more sense to just add AbstractThreadPool to the whitelist?

These executors were added to the whitelist to help resolve this issue: when a request is made to the server that then triggers a network request from the client, two traces are created instead of a single trace with the server span as the parent and the client span as the child. However, the problem still persists. Do I need to check that no executors related to the client have been skipped?

...rc/main/java/datadog/trace/instrumentation/mulehttpconnector/client/ClientRequestAdvice.java

heathkd · 2020-04-20T14:22:41Z

...c/main/java/datadog/trace/instrumentation/mulehttpconnector/client/ClientResponseAdvice.java

+    final ContextStore<AsyncCompletionHandler, AgentSpan> contextStore =
+        InstrumentationContext.get(AsyncCompletionHandler.class, AgentSpan.class);
+    final AgentSpan span = contextStore.get(handler);
+    final AgentScope scope = activateSpan(span, true);


This is in-line with the comment in ClientRequestAdvice. Should the span be reactivated here?

heathkd · 2020-04-20T14:26:22Z

...0/src/main/java/datadog/trace/instrumentation/mulehttpconnector/clientV2/ClientV2Advice.java

@@ -0,0 +1,36 @@
+package datadog.trace.instrumentation.mulehttpconnector.clientV2;


Classes in the "clientV2" package can be ignored during review. Please review the classes in the "client" package instead. I was experimenting with slightly different instrumentation for the client.

heathkd · 2020-04-20T14:26:30Z

...rc/main/java/datadog/trace/instrumentation/mulehttpconnector/clientV2/ClientV2Decorator.java

@@ -0,0 +1,38 @@
+package datadog.trace.instrumentation.mulehttpconnector.clientV2;


Classes in the "clientV2" package can be ignored during review. Please review the classes in the "client" package instead. I was experimenting with slightly different instrumentation for the client.

heathkd · 2020-04-20T14:26:41Z

...n/java/datadog/trace/instrumentation/mulehttpconnector/clientV2/ClientV2Instrumentation.java

@@ -0,0 +1,40 @@
+package datadog.trace.instrumentation.mulehttpconnector.clientV2;


Classes in the "clientV2" package can be ignored during review. Please review the classes in the "client" package instead. I was experimenting with slightly different instrumentation for the client.

heathkd · 2020-04-20T14:35:33Z

...r-1.0/src/main/java/datadog/trace/instrumentation/mulehttpconnector/server/ServerAdvice.java

+      final AgentSpan span = startSpan("mule.http.server", parentContext);
+
+      final AgentScope scope = activateSpan(span, false);
+      scope.setAsyncPropagation(true);


When a request is made to the server that then triggers a network request from the client, two traces are created instead of a single trace with the server span as the parent and the client span as the child. I have set asyncPropagation to true, but I have also closed the scope. Would that result in the disconnected server and client spans?

heathkd · 2020-04-20T14:40:44Z

dd-java-agent/instrumentation/mule-http-connector-1.0/src/test/groovy/MuleHttpServerTest.groovy

+      return ctx.getInvokeAction();
+    }
+
+    public HttpResponsePacket buildResponse(HttpRequestPacket request) {


This needs to be updated to call the connector method. It should also set SUCCESS.body to the body of the response instead of the reasonPhrase. However, it is unclear how to do this. It may be possible to write the response body using ctx.write().

heathkd · 2020-04-23T23:47:00Z

...main/java/datadog/trace/instrumentation/mulehttpconnector/filterchain/FilterchainAdvice.java

+    }
+
+    final AgentScope scope =
+        activateSpan((AgentSpan) ctx.getAttributes().getAttribute(SPAN), false);


The span is activated and the scope is explicitly closed. However, the logs point to an error where the tracer is attempting to close a specific scope while the current scope is "null". This code may be the cause for that error.

… span

.../instrumentation/mule-http-connector-1.0/src/test/groovy/GrizzlyFilterchainServerTest.groovy

...va-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/Pair.java

tylerbenson

Looks like you already resolved some of these comments. I'm going to submit now and continue reviewing.

dd-java-agent/instrumentation/grizzly-client-1.1/grizzly-client-1.1.gradle

...y-client-1.1/src/main/java/datadog/trace/instrumentation/grizzly/client/ClientDecorator.java

...src/main/java/datadog/trace/instrumentation/grizzly/client/ClientRequestInstrumentation.java

dd-java-agent/instrumentation/grizzly-client-1.1/grizzly-client-1.1.gradle

devinsba · 2020-05-15T14:36:08Z

dd-java-agent/instrumentation/grizzly-2/src/test/groovy/GrizzlyTest.groovy

@@ -43,7 +44,7 @@ class GrizzlyTest extends HttpServerTest<HttpServer> {

  @Override
  String expectedOperationName() {
-    return "grizzly.request"
+    return DDSpanNames.GRIZZLY_REQUEST


Did you want a static import here

dd-java-agent/instrumentation/grizzly-client-1.1/grizzly-client-1.1.gradle

devinsba · 2020-05-15T14:42:09Z

...ient-1.1/src/main/java/datadog/trace/instrumentation/grizzly/client/ClientRequestAdvice.java

+  @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
+  public static void onExit(@Advice.Enter final AgentScope scope) {
+    // span closed in ClientResponseAdvice, scope only created for profiler's benefit
+    scope.close();


We tend to do a null check on these but I don't think it could be null here

devinsba · 2020-05-15T16:12:14Z

dd-trace-core/src/main/java/datadog/trace/core/StringTables.java

@@ -36,6 +37,7 @@
    internConstantsUTF8(Tags.class);
    internConstantsUTF8(DDSpanTypes.class);
    internConstantsUTF8(DDComponents.class);
+    internConstantsUTF8(DDSpanNames.class);


We might want a follow-up PR that goes through all the others and moves them here

fill your boots :)

tylerbenson

A few minor questions and comments remaining then I think we can merge.

...ient-1.9/src/main/java/datadog/trace/instrumentation/grizzly/client/ClientRequestAdvice.java

tylerbenson · 2020-05-15T15:39:10Z

...ent-1.9/src/main/java/datadog/trace/instrumentation/grizzly/client/ClientResponseAdvice.java

+      @Advice.Argument(0) final Response response) {
+    ContextStore<AsyncCompletionHandler, Pair> contextStore =
+        InstrumentationContext.get(AsyncCompletionHandler.class, Pair.class);
+    Pair<AgentSpan, AgentSpan> spanWithParent = contextStore.get(handler);


It's easy to lose track of what's left and what's right below... consider assigning them to more descriptive local variables early.

...ain/java/datadog/trace/instrumentation/grizzlyhttp232/DefaultFilterChainInstrumentation.java

...http-2.3.20/src/main/java/datadog/trace/instrumentation/grizzlyhttp232/GrizzlyDecorator.java

settings.gradle

tylerbenson · 2020-05-15T16:50:43Z

sorry, that was an accident.

tylerbenson · 2020-05-15T17:36:54Z

dd-java-agent/instrumentation/grizzly-client-1.9/grizzly-client-1.9.gradle

+  compileOnly group: 'org.glassfish.grizzly', name: 'grizzly-http-client', version: '1.12'
+  testCompile group: 'org.glassfish.grizzly', name: 'grizzly-http-client', version: '1.12'


Please set the compileOnly to 1.9 and add a comment saying that the tests only seem to work with 1.12.

Done. Would be good to get to the bottom of this one.

* Define packages in exporter class loader (#409) * Update docs about needing java 11 to build (#412) * Update version to 0.3.0 (#413) * Update the README (#414) * Move build and configure to top as getting started section * Add manual instrumentation section * Document `@WithSpan` annotation * Move developer specific information to CONTRIBUTING.md * Cleanup formatting and use consistent spacing * Update version to 0.4.0-SNAPSHOT (#415) * Update CONTRIBUTING.md back to Java 11 (#419) * Add Zipkin exporter support (#411) * #375 Add Zipkin exporter support Signed-off-by: Sergei Malafeev <sergeymalafeev@gmail.com> * #375 use OkHttpSender for Zipkin exporter Signed-off-by: Sergei Malafeev <sergeymalafeev@gmail.com> * #375 add Zipkin exporter to README Signed-off-by: Sergei Malafeev <sergeymalafeev@gmail.com> Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> * Remove inactive maintainer (#420) * Fix brolen anchor link (#422) * Fix khttp instrumentation in case of absent or read-only headers map (#416) * Add new approver (#429) * Fix sqlNormalizerEnabled initialization (#432) * Remove unused config (#424) * Change names of servlet based server spans (#428) * Add documentation describing non-obvious points of Servlet instrumentations * Change names of servlet based server spans * Fix java google format (#439) * Remove deprecated usage from internal instrumentation (DataDog/dd-trace-java#1441) * Update gradle to 6.4 (DataDog/dd-trace-java#1443) * Migrate lettuce instrumentation away from deprecated finishSpanOnClose (DataDog/dd-trace-java#1445) * Make Retrys consistent (DataDog/dd-trace-java#1442) * Rename java packages for lettuce 4 and 5 to not have collisions (DataDog/dd-trace-java#1450) * Adding an option to manually disable Kafka headers (DataDog/dd-trace-java#1448) * Add version specific names to allow disabling only a specific version (DataDog/dd-trace-java#1456) * Attempt to improve muzzle time by randomly ignoring versions until 10 remain (DataDog/dd-trace-java#1451) * Save circle cache with name matching restore patterns (DataDog/dd-trace-java#1457) * Wrap log statements using varargs to avoid object allocation (DataDog/dd-trace-java#1466) * Grizzly-http and grizzly-client instrumentation (DataDog/dd-trace-java#1365) * More refactoring for ScopeManager (DataDog/dd-trace-java#1467) * Remove Java 9 and 10 from the build (DataDog/dd-trace-java#1473) * Disable CI cache for muzzle (DataDog/dd-trace-java#1479) * Separate out core instrumentation for AWS SDK to allow manual setup o… (#421) * Separate out core instrumentation for AWS SDK to allow manual setup of instrumentation. * Instrumentation core test * Fix sporadic Elasticsearch test failures (#444) * Fix sporadic grizzly test failure (#448) * Fixes integration with latest version of Finatra (#450) * #425 allow only one propagator per concern (#451) * Updates to reflect new repo name (#454) * Remove printlns that were accidentally committed (#459) * Remove unnecessary version constant (#455) Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> Co-authored-by: Steve Flanders <steve@omnition.io> Co-authored-by: Sergei Malafeev <sergeymalafeev@gmail.com> Co-authored-by: Tyler Benson <tyler.benson@datadoghq.com> Co-authored-by: Nikolay Martynov <mar.kolya@gmail.com> Co-authored-by: Brian Devins-Suresh <brian.devinssuresh@datadoghq.com> Co-authored-by: Richard Startin <richard.startin@datadoghq.com> Co-authored-by: Anuraag Agrawal <anuraaga@gmail.com>

heathkd requested a review from a team as a code owner April 13, 2020 16:03

heathkd self-assigned this Apr 13, 2020

heathkd requested review from tylerbenson, devinsba and dougqh April 13, 2020 16:04

heathkd changed the title ~~Mulesoft instrumentation~~ WIP Mulesoft instrumentation Apr 13, 2020

heathkd force-pushed the heather.dsouza/mulesoft-instrumentation branch 6 times, most recently from 2b8c788 to 5e26ea1 Compare April 19, 2020 19:30

heathkd commented Apr 20, 2020

View reviewed changes

...rc/main/java/datadog/trace/instrumentation/mulehttpconnector/client/ClientRequestAdvice.java Outdated Show resolved Hide resolved

heathkd commented Apr 20, 2020

View reviewed changes

heathkd force-pushed the heather.dsouza/mulesoft-instrumentation branch from e40a1a6 to 858f717 Compare April 21, 2020 14:09

heathkd commented Apr 23, 2020

View reviewed changes

heathkd added 7 commits April 24, 2020 11:50

set-up instrumentation module

32928ec

update decorate with request and response objects

b940c76

initial HttpListener instrumentation

1041484

initial HttpRequester instrumentation

c08b1cc

create client span from mule http client

6860ad0

remove http listener instrumentation - this will be handled in server…

75e2c47

… span

reorganize and rename client instrumentation classes

40f4c0f

post merge fix

3c9d105

richardstartin force-pushed the heather.dsouza/mulesoft-instrumentation branch from 71f1216 to 3c9d105 Compare April 30, 2020 22:43

richardstartin added 3 commits April 30, 2020 23:47

address basic comments before moving modules

24e9ab2

rename filterchain instrumentation

28c4454

group filterchain and server, remove ContextAttributes

0ed2f02

tylerbenson reviewed May 1, 2020

View reviewed changes

.../instrumentation/mule-http-connector-1.0/src/test/groovy/GrizzlyFilterchainServerTest.groovy Outdated Show resolved Hide resolved

Merge branch 'master' into heather.dsouza/mulesoft-instrumentation

154ae82

dougqh reviewed May 5, 2020

View reviewed changes

...va-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/api/Pair.java Outdated Show resolved Hide resolved

Merge branch 'master' into heather.dsouza/mulesoft-instrumentation

dc170af

richardstartin force-pushed the heather.dsouza/mulesoft-instrumentation branch from ca7ab34 to 9e0a743 Compare May 14, 2020 09:18

richardstartin changed the title ~~Mulesoft instrumentation~~ Grizzly-http and grizzly-client instrumentation May 15, 2020

richardstartin force-pushed the heather.dsouza/mulesoft-instrumentation branch from a74d603 to baab018 Compare May 15, 2020 15:20

tylerbenson reviewed May 15, 2020

View reviewed changes

devinsba reviewed May 15, 2020

View reviewed changes

tylerbenson reviewed May 15, 2020

View reviewed changes

tylerbenson closed this May 15, 2020

tylerbenson deleted the heather.dsouza/mulesoft-instrumentation branch May 15, 2020 16:50

tylerbenson restored the heather.dsouza/mulesoft-instrumentation branch May 15, 2020 16:50

tylerbenson reopened this May 15, 2020

address review comments

4b9af37

richardstartin force-pushed the heather.dsouza/mulesoft-instrumentation branch from b8feb7b to 4b9af37 Compare May 15, 2020 17:23

tylerbenson approved these changes May 15, 2020

View reviewed changes

clarify grizzly-client version mismatch

a802476

richardstartin merged commit ee191b1 into master May 18, 2020

richardstartin deleted the heather.dsouza/mulesoft-instrumentation branch May 18, 2020 08:59

github-actions bot added this to the 0.52.0 milestone May 18, 2020

trask mentioned this pull request Jun 29, 2020

Simplify grizzly instrumentation, Part 1 open-telemetry/opentelemetry-java-instrumentation#604

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grizzly-http and grizzly-client instrumentation #1365

Grizzly-http and grizzly-client instrumentation #1365

heathkd commented Apr 13, 2020

heathkd commented Apr 13, 2020

heathkd Apr 20, 2020 •

edited

Loading

heathkd Apr 20, 2020

heathkd Apr 20, 2020 •

edited

Loading

heathkd Apr 20, 2020

heathkd Apr 20, 2020

heathkd Apr 20, 2020

heathkd Apr 20, 2020

heathkd Apr 20, 2020

heathkd Apr 23, 2020

tylerbenson left a comment

devinsba May 15, 2020

devinsba May 15, 2020

devinsba May 15, 2020

richardstartin May 15, 2020

tylerbenson left a comment

tylerbenson May 15, 2020

tylerbenson commented May 15, 2020

tylerbenson May 15, 2020

richardstartin May 15, 2020

		@@ -0,0 +1,36 @@
		package datadog.trace.instrumentation.mulehttpconnector.clientV2;

		@@ -0,0 +1,38 @@
		package datadog.trace.instrumentation.mulehttpconnector.clientV2;

		@@ -0,0 +1,40 @@
		package datadog.trace.instrumentation.mulehttpconnector.clientV2;

		compileOnly group: 'org.glassfish.grizzly', name: 'grizzly-http-client', version: '1.12'
		testCompile group: 'org.glassfish.grizzly', name: 'grizzly-http-client', version: '1.12'

Grizzly-http and grizzly-client instrumentation #1365

Grizzly-http and grizzly-client instrumentation #1365

Conversation

heathkd commented Apr 13, 2020

heathkd commented Apr 13, 2020

heathkd Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heathkd Apr 20, 2020 • edited Loading

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

tylerbenson 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

tylerbenson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerbenson commented May 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heathkd Apr 20, 2020 •

edited

Loading

heathkd Apr 20, 2020 •

edited

Loading