WIP: Implemetation Issue#35 #42

kameshsampath · 2017-09-05T13:57:16Z

fixes #35

malafeev · 2017-09-05T14:08:02Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/async/AsyncDefaultAutoConfiguration.java

+ * @author kamesh
+ */
+@Configuration
+@EnableAsync


should we enable async? I think user should decide to enable/disable spring features.

The same question, although sleuth enables it here too.

i had the same doubt when doing it, Sleuth does it so added it but then I had another thought is that unless @Async added this will not be triggered, so when I do @Async then I need to @EnabledAsync

malafeev · 2017-09-05T14:11:46Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    private static final String ASYNC_COMPONENT = "async";
+    public static final String TAG_LC = "localcomponent";
+    public static final String TAG_CLASS = "class";
+    public static final String TAG_METHOD = "method";


shouldn't be all these strings private?

malafeev · 2017-09-05T14:12:17Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    public static final String TAG_CLASS = "class";
+    public static final String TAG_METHOD = "method";
+
+    Tracer tracer;


shouldn't be private final?

malafeev · 2017-09-05T14:13:03Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+
+    Tracer tracer;
+    protected final ActiveSpan.Continuation continuation;
+    protected final ActiveSpan activeSpan;


shouldn't be both private?

pavolloffay · 2017-09-05T14:13:42Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    public static final String TAG_METHOD = "method";
+
+    Tracer tracer;
+    protected final ActiveSpan.Continuation continuation;


Is this aspect singleton or instance per-invocation?

this is per invocation? why?

pavolloffay · 2017-09-05T14:14:19Z

pom.xml

@@ -123,6 +123,12 @@
      </dependency>

      <dependency>
+        <groupId>io.opentracing.contrib</groupId>
+        <artifactId>opentracing-concurrent</artifactId>
+        <version>0.0.1</version>


export version to property.

pavolloffay · 2017-09-05T14:15:21Z

.editorconfig

@@ -0,0 +1,18 @@
+# top-most EditorConfig file


please do not push this file

and also .gitkeep

.gitkeep is fine, why .editorconfig - helps accidental formats done on the code and we can maintain the standard formatting.. now adding to .gitkeep but IMHO worth having it

pavolloffay · 2017-09-05T14:23:10Z

opentracing-spring-cloud-starter/pom.xml

@@ -6,7 +6,7 @@
  <parent>
    <groupId>io.opentracing.contrib</groupId>
    <artifactId>opentracing-spring-cloud-parent</artifactId>
-    <version>0.0.1-SNAPSHOT</version>


you need probably to rebase off master.

There is still a change here and it should not be.

the same in other pom files

pavolloffay · 2017-09-05T14:24:52Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/async/AsyncDefaultAutoConfiguration.java

+ * @author kamesh
+ */
+@Configuration
+@EnableAsync


The same question, although sleuth enables it here too.

pavolloffay · 2017-09-05T14:30:12Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+        this.continuation.activate();
+
+        Span span = this.tracer.buildSpan(operationName(pjp))
+            .asChildOf(this.activeSpan.context())


If there is an active span it will be automatically considered as parent. This line can be removed

pavolloffay · 2017-09-05T14:30:50Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+            .asChildOf(this.activeSpan.context())
+            .startManual();
+
+        this.activeSpan.setTag(TAG_LC, ASYNC_COMPONENT);


Please set all this tags directly when calling tracer.buildSpan("foo").tag()

pavolloffay · 2017-09-05T14:32:12Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    @Autowired
+    public TraceAsyncAspect(Tracer tracer) {
+        this.tracer = tracer;
+        this.activeSpan = tracer.activeSpan();


this is not needed just capture the continuation.

but how do I capture continuation without activeSpan ??

just capture a continuation then when you activate continuation it will set that span as active.

if I get you correctly you don't want that extra member there for activeSpan

I mean, just get activeSpan capture continuation but do not store the active span in the class.

pavolloffay · 2017-09-05T14:33:35Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    public Object traceBackgroundThread(final ProceedingJoinPoint pjp) throws Throwable {
+        this.continuation.activate();
+
+        Span span = this.tracer.buildSpan(operationName(pjp))


if operationName(pjp) throws an exception the span won't be closed. I would move it to the try block

pavolloffay · 2017-09-05T15:33:27Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+        Span span = null;
+        try {
+            span = this.tracer.buildSpan(operationName(pjp))
+                .withTag(TAG_LC, ASYNC_COMPONENT)


use sandard OT tag Tags.Component. https://github.com/opentracing/specification/blob/master/semantic_conventions.yaml#L10

done! thanks for pointing out

pavolloffay · 2017-09-05T15:33:57Z

pom.xml

@@ -46,6 +46,7 @@
    <version.io.opentracing>0.30.0</version.io.opentracing>
    <version.io.opentracing.contrib-opentracing-spring-web>0.0.8</version.io.opentracing.contrib-opentracing-spring-web>
    <version.io.opentracing.contrib-opentracing-spring-jms>0.0.3</version.io.opentracing.contrib-opentracing-spring-jms>
+    <version.io.opentracing.contrib--java-concurrent>0.0.1</version.io.opentracing.contrib--java-concurrent>


there is double -``-

pavolloffay · 2017-09-06T07:46:10Z

pom.xml

@@ -47,6 +47,7 @@
    <version.io.opentracing.contrib-opentracing-spring-web>0.0.8</version.io.opentracing.contrib-opentracing-spring-web>
    <version.io.opentracing.contrib-opentracing-spring-jms>0.0.3</version.io.opentracing.contrib-opentracing-spring-jms>
    <version.io.opentracing.contrib-java-jdbc>0.0.3</version.io.opentracing.contrib-java-jdbc>
+    <version.io.opentracing.contrib--java-concurrent>0.0.1</version.io.opentracing.contrib--java-concurrent>


there is still double -- in the name

fixed in latest commit

I see that it's still here

pavolloffay · 2017-09-06T07:46:51Z

...ud/src/main/java/io/opentracing/contrib/spring/cloud/async/AsyncCustomAutoConfiguration.java

+@ConditionalOnBean(AsyncConfigurer.class)
+@AutoConfigureBefore(AsyncDefaultAutoConfiguration.class)
+//TODO when Scheduling is added we need to do @AutoConfigurationAfter here
+public class AsyncCustomAutoConfiguration implements BeanPostProcessor {


this has to be added to spring factories file. (located in resources)

sorry missed in previous commit, added to new commit

Could you please add class level comment what it does, what it traces?

…uration to spring.factories

pavolloffay · 2017-09-06T13:11:57Z

opentracing-spring-cloud-starter/pom.xml

@@ -1,112 +1,114 @@
 <?xml version="1.0" encoding="UTF-8"?>
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">


could you please revert changes in this file which are not related to this PR?

sorry about, thought i had reverted in previous commit, it was because of the test changes introduced earlier using zipkin for IT

pavolloffay · 2017-09-06T13:13:30Z

...-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/LazyTraceExecutor.java

+    }
+
+    @Override
+    public void execute(Runnable command) {


Why overriding only this method?

pavolloffay · 2017-09-06T13:14:38Z

...g-spring-cloud/src/main/java/io/opentracing/contrib/spring/cloud/async/TraceAsyncAspect.java

+    }
+
+    private String operationName(ProceedingJoinPoint pjp) {
+        return getMethod(pjp, pjp.getTarget()).getName();


is this safe? Can this throw NPE?

I dont think so - i cant have a "@aspect" on a method whose name can be null??

pavolloffay · 2017-09-06T13:16:23Z

...ng-cloud/src/test/java/io/opentracing/contrib/spring/cloud/async/AsyncTestConfiguration.java

+
+@Configuration
+@EnableAsync
+@ComponentScan("io.opentracing.contrib.spring.cloud.async")


Why do we need this?

just to make the test bit efficient, and confined only to Async testing, otherwise it tends to load a whole lot from classpath which is not required for this test context

pavolloffay · 2017-09-06T13:17:12Z

pom.xml

@@ -47,6 +47,7 @@
    <version.io.opentracing.contrib-opentracing-spring-web>0.0.8</version.io.opentracing.contrib-opentracing-spring-web>
    <version.io.opentracing.contrib-opentracing-spring-jms>0.0.3</version.io.opentracing.contrib-opentracing-spring-jms>
    <version.io.opentracing.contrib-java-jdbc>0.0.3</version.io.opentracing.contrib-java-jdbc>
+    <version.io.opentracing.contrib--java-concurrent>0.0.1</version.io.opentracing.contrib--java-concurrent>


I see that it's still here

pavolloffay · 2017-09-06T14:24:15Z

...d/src/main/java/io/opentracing/contrib/spring/cloud/async/AsyncDefaultAutoConfiguration.java

+
+        @Override
+        public Executor getAsyncExecutor() {
+            return new LazyTraceExecutor(this.beanFactory, new SimpleAsyncTaskExecutor());


I think it would be better to directly return TracerExecutorService from ot-concurrent

pavolloffay · 2017-09-06T14:31:37Z

...ing-cloud/src/test/java/io/opentracing/contrib/spring/cloud/async/LazyTraceExecutorTest.java

+    BeanFactory beanFactory;
+
+    @Mock
+    AsyncConfigurer asyncConfigurer;


It's not used here

it will be used by "@Injectmocks"

pavolloffay · 2017-09-06T14:32:00Z

...ing-cloud/src/test/java/io/opentracing/contrib/spring/cloud/async/LazyTraceExecutorTest.java

+public class LazyTraceExecutorTest {
+
+    @Mock
+    BeanFactory beanFactory;


not used here

it will be used by "@Injectmocks"

pavolloffay · 2017-09-06T14:35:18Z

opentracing-spring-cloud/src/test/java/io/opentracing/contrib/spring/cloud/async/AsyncTest.java

+        List<MockSpan> finishedSpans = mockTracer.finishedSpans();
+        assertThat(finishedSpans.size()).isEqualTo(1);
+        MockSpan mockSpan = mockTracer.finishedSpans().get(0);
+        assertThat(mockSpan).isNotNull();


this is redundant

pavolloffay · 2017-09-06T14:38:03Z

opentracing-spring-cloud/src/test/java/io/opentracing/contrib/spring/cloud/async/AsyncTest.java

+    }
+
+    @Test
+    public void testAsyncTraceAndSpans() throws Exception {


Could you please explain why there is @Before and one test, in the before you execute traced interface but you assert in the test. I think it should be merged.

Most important is a wiring test. e.g. create active span before calling the delayer and then verify that reported spans are in the same trace.

i was having it when I had multiple tests, now its not needed will remove

pavolloffay · 2017-09-06T14:53:09Z

We also need a "wiring" test where a callable is returned from the REST controller. Inside the callable create a span and verify that server span and span from callable are in the same trace.

edit: needed for this use cases https://github.com/redhat-developer-demos/brewery/blob/iteration-3/brewing/src/main/java/io/spring/cloud/samples/brewery/aggregating/IngredientsController.java#L54

Handles instrumentation for * Async annotated methods * WebAsyncTask return types method for RestController/Controller * Callable return types method for RestController/Controller

pavolloffay · 2017-09-20T14:52:06Z

closing implemented in #43 , @kameshsampath thanks a lot!

malafeev reviewed Sep 5, 2017

View reviewed changes

pavolloffay reviewed Sep 5, 2017

View reviewed changes

Kamesh Sampath added 2 commits September 5, 2017 21:37

WIP - @async implementation

03e4db6

WIP: Issue#35, fixing comments from @malafeev and @pavolloffay

22b6c00

kameshsampath force-pushed the issue-35 branch from 295f350 to 22b6c00 Compare September 5, 2017 16:09

pavolloffay reviewed Sep 6, 2017

View reviewed changes

Kamesh Sampath added 2 commits September 6, 2017 13:51

Issue#35 - Added Unit and Integation Tests

d00faf9

Added io.opentracing.contrib.spring.cloud.async.AsyncCustomAutoConfig…

0f4b6df

…uration to spring.factories

pavolloffay reviewed Sep 6, 2017

View reviewed changes

Kamesh Sampath added 3 commits September 6, 2017 20:36

Accidentally caused by previvous commit

4c2e918

WIP - Async Task - Test Cases

16c68ad

Resolving Issue#35 .,

b5fc841

Handles instrumentation for * Async annotated methods * WebAsyncTask return types method for RestController/Controller * Callable return types method for RestController/Controller

pavolloffay mentioned this pull request Sep 8, 2017

@Async, AsyncWebTask and Executor instrumentation #43

Merged

kameshsampath mentioned this pull request Sep 14, 2017

Add instrumentations #49

Closed

pavolloffay closed this Sep 20, 2017

mdvorak mentioned this pull request Apr 20, 2018

.editorconfig in project #130

Closed

		@@ -1,112 +1,114 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">

WIP: Implemetation Issue#35 #42

WIP: Implemetation Issue#35 #42

Conversation

kameshsampath commented Sep 5, 2017 • edited by pavolloffay

Choose a reason for hiding this comment

pavolloffay Sep 5, 2017 • edited

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

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

Choose a reason for hiding this comment

pavolloffay Sep 5, 2017 • edited

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

kameshsampath Sep 6, 2017 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Sep 6, 2017 • edited

pavolloffay commented Sep 20, 2017

kameshsampath commented Sep 5, 2017 •

edited by pavolloffay

pavolloffay Sep 5, 2017 •

edited

pavolloffay Sep 5, 2017 •

edited

kameshsampath Sep 6, 2017 •

edited

pavolloffay commented Sep 6, 2017 •

edited