add OpenTracing-Examples #174

malafeev · 2017-09-05T04:09:45Z

move examples from https://github.com/opentracing-contrib/java-examples to be part of
opentracing-java

coveralls · 2017-09-05T04:14:01Z

Coverage remained the same at 74.074% when pulling d20f546 on malafeev:master into 578d4e7 on opentracing:master.

yurishkuro · 2017-09-05T04:48:33Z

could we add a readme at the module level briefly explaining what each example represents?

coveralls · 2017-09-05T06:37:19Z

Coverage remained the same at 74.074% when pulling 744ebd8 on malafeev:master into 578d4e7 on opentracing:master.

objectiser · 2017-09-05T08:04:25Z

Instead of just moving them into this repo as "examples", do we want to take this opportunity to start some form of conformance test suite? Could be based on these examples initially and changed over time if necessary to ensure testing all aspects of the API.

malafeev · 2017-09-07T07:31:07Z

These examples could evolve into

conformance test suite

some day.
Now it is more for testing different api proposals (like get rid of Continuation :) ).
If somebody proposes such great change let's make examples work first :)

carlosalberto · 2017-09-07T18:06:41Z

some day. Now it is more for testing different api proposals

I agree on this point with @malafeev - I think at this time they are a testbed for such changes, and not much of actual tests (and thus the listing of specific use cases there, and not the common ones).

tedsuo · 2017-09-08T21:01:15Z

I like the idea @objectiser; I'd like to see us build out a test harness for tracers to verify that they conform to the spec properly. Perhaps durning or after the release candidate process we can work on this?

The immediate concern that examples were created to solve is that we need a place to discuss the API using code. If we encounter a usecase during the release candidate process where we feel the API is incomplete or ambiguous, we need a way to record this fact for all to see. So this is about "correctness of the API" itself as opposed to "correctness of an implementation of the API".

Personally, I think we can continue to push and change this example/test code as patch versions to the API, so we don't need to get it perfect on the first shot and can merge this in. If that approach is problematic, we should leave it in ot-contrib until we work it out.

pavolloffay

I did a partial review. I would like to see more comments, why something is bad and something good.

Could we get rid of all unnecessary waits/sleeps and joins with magic constants? It is just a distraction.

pavolloffay · 2017-09-11T08:52:39Z

opentracing-examples/pom.xml

+        </dependency>
+
+        <dependency>
+            <groupId>com.jayway.awaitility</groupId>


Use new version https://mvnrepository.com/artifact/org.awaitility/awaitility/3.0.0 (it's under slightly different groupId/artifactId)

Please define version in properties

pavolloffay · 2017-09-11T08:56:03Z

opentracing-examples/pom.xml

+        </dependency>
+
+        <dependency>
+            <groupId>ch.qos.logback</groupId>


can we use just System.out.print() for simplicity?

yes, but I wanted to print thread name, time, don't use ugly string concatenation...

pavolloffay · 2017-09-11T09:07:04Z

opentracing-examples/src/test/java/io/opentracing/examples/TestUtils.java

+            }
+        }
+        if (found.size() > 1) {
+            throw new RuntimeException("Ups, it's too much");


A more meaningful message would be good

IllegalArgumentException is more suitable for this

pavolloffay · 2017-09-11T09:13:44Z

opentracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/Callback.java

+        ActiveSpan activeSpan = continuation.activate();
+
+        try {
+            TimeUnit.SECONDS.sleep(1);


Is this necessary? Just leave a comment in the middle that it wraps some logic.

added commend

pavolloffay · 2017-09-11T09:20:21Z

...tracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/TestCallback.java

+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.Assert.assertEquals;
+
+public class TestCallback {


Could you please use the Test suffix like it is in other tests in this repo?

pavolloffay · 2017-09-11T14:20:07Z

...acing-examples/src/test/java/io/opentracing/examples/common_request_handler/TestHandler.java

+        }
+
+        assertNotEquals(finished.get(0).context().traceId(), finished.get(1).context().traceId());
+        assertEquals(finished.get(0).parentId(), finished.get(1).parentId());


I think that these two last lines are distracting. I would rather see asset on 0 it's more explicit.

pavolloffay · 2017-09-11T14:25:36Z

opentracing-examples/src/test/java/io/opentracing/examples/common_request_handler/Client.java

+    }
+
+
+    public Future<Object> send(final Object message) {


nit: return String

pavolloffay · 2017-09-11T14:26:32Z

...acing-examples/src/test/java/io/opentracing/examples/common_request_handler/TestHandler.java

+    }
+
+    /**
+     * Solution is bad because parent is per client (we don't have better choice)


Explain it better, that happened, why it is bad

pavolloffay · 2017-09-11T14:27:58Z

...cing-examples/src/test/java/io/opentracing/examples/late_span_finish/TestLateSpanFinish.java

+
+        for (int i = 0; i < 2; i++) {
+            assertEquals(true, spans.get(2).finishMicros() >= spans.get(i).finishMicros());
+            assertEquals(spans.get(2).context().traceId(), spans.get(i).context().traceId());


nit: create TestUtils.assertSameTrace(List<MockSpan>)

pavolloffay · 2017-09-11T14:29:53Z

opentracing-examples/README.md

+
+Examples of common instrumentation patterns:
+
+- **activate_deactivate** - callbacks finish at some time.   


Could we create some categories? e.g. one related to client-server other to callbacks and order them from the simplest to more complex use cases?

coveralls · 2017-09-12T02:13:11Z

Coverage remained the same at 74.074% when pulling 2fc00b0 on malafeev:master into 431ebc3 on opentracing:master.

coveralls · 2017-09-12T02:52:53Z

Coverage remained the same at 74.074% when pulling cce6752 on malafeev:master into 431ebc3 on opentracing:master.

pavolloffay · 2017-09-12T10:07:15Z

@malafeev I have a meta request. Could you please clean the sources:

do not use .join(SOME_MAGIC_NUMBER)
do not use future.get(MAGIC, time_unit)
use comments consistently (do not mix /**/ // and pay attention to indentation).
do not use sleep(ms) when it's not needed. In the most places, it's not needed. It's just a noise which makes it harder to read, nothing more.

pavolloffay · 2017-09-12T08:46:42Z

opentracing-examples/pom.xml

+        <dependency>
+            <groupId>ch.qos.logback</groupId>
+            <artifactId>logback-classic</artifactId>
+            <version>1.2.3</version>


Define versions in parent pom

pavolloffay · 2017-09-12T08:48:39Z

opentracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/Callback.java

+import java.util.concurrent.TimeUnit;
+
+/**
+ * Callback which executed at some time. We don't know when it is started, when it is


I think this comment is misleading. It depends on how the callback is executed.

Idea is to show case when callback is executed at some time and we don't know when it is started, completed, no status.

My point is that this comment is at wrong place.

pavolloffay · 2017-09-12T08:50:54Z

...tracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/TestCallback.java

+    public void test() throws Exception {
+        Thread entryThread = entryThread();
+        entryThread.start();
+        entryThread.join(10_000);


Why this magic number?Applies to a lot of places in this PR.

That thread will end, no need to specify max wait time

pavolloffay · 2017-09-12T08:55:15Z

...tracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/TestCallback.java

+    private int getTestTagsCount(MockSpan mockSpan) {
+        Map<String, Object> tags = mockSpan.tags();
+        int tagCounter = 0;
+        for (String tagKey : tags.keySet()) {


nit: mockSpan.tags().keySet()

pavolloffay · 2017-09-12T09:03:23Z

...examples/src/test/java/io/opentracing/examples/multiple_callbacks/TestMultipleCallbacks.java

+            assertEquals(parentSpan.context().spanId(), spans.get(i).parentId());
+        }
+
+        assertNull(tracer.activeSpan());


This is just redundant noise. Are you trying to test active span source in MockTracer works?

maybe client activated span and not deactivated? better to test than assume.

pavolloffay · 2017-09-12T09:43:00Z

...acing-examples/src/test/java/io/opentracing/examples/common_request_handler/TestHandler.java

+    }
+
+    private static MockSpan getOneByOperationName(List<MockSpan> spans, String name) {
+        List<MockSpan> found = new ArrayList<>();


you don't need a list to do this.

pavolloffay · 2017-09-12T09:43:11Z

...acing-examples/src/test/java/io/opentracing/examples/common_request_handler/TestHandler.java

+            }
+        }
+        if (found.size() > 1) {
+            throw new RuntimeException("Ups, it's too much");


IllegalArgument ex is better

pavolloffay · 2017-09-12T09:58:43Z

...cing-examples/src/test/java/io/opentracing/examples/late_span_finish/TestLateSpanFinish.java

+                    try (ActiveSpan childSpan1 = tracer.buildSpan("task1").startActive()) {
+                        sleep(55);
+                    }
+                    span.capture(); // Workaround, prevent parentSpan from being finished here.


This is anti-pattern. Do we want to have anti-patterns in examples? There should be continuation create for each runnable. e.g. https://paste.fedoraproject.org/paste/SvU3kpGwJbMqP2HTI6mi9A

The idea for this test sample was to have a 'master' Span whose lifetime is not tied to any children - hence we couldn't pass a Continuation, as creating them would increment the refcount for them.

Then again, another option to prevent this usage would be to simply have them create a Continuation and don't close the 'master' ActiveSpan (which, I think, you will definitely prefer ;) )

These examples need more explanatory comments. Especially for this anti-patterns, explain why it is written like it is.

pavolloffay · 2017-09-12T09:59:04Z

...cing-examples/src/test/java/io/opentracing/examples/late_span_finish/TestLateSpanFinish.java

+                // Alternative to calling makeActive() is to pass it manually to asChildOf() for each created Span.
+                try (ActiveSpan span = tracer.makeActive(parentSpan)) {
+                    try (ActiveSpan childSpan1 = tracer.buildSpan("task1").startActive()) {
+                        sleep(55);


you don't really need a sleep here

pavolloffay · 2017-09-12T10:08:51Z

opentracing-examples/README.md

+- **late_span_finish** - late parent span finish
+- **listener_per_request** - one listener per request
+- **multiple_callbacks** - multiple callbacks
+- **nested_callbacks** - nested callbacks


Create a section for async/callbacks. Better comments would be appreciated.

multiple_callbacks - multiple callbacks`. e.g. how it differs from nested ones.

bhs

(thanks!)

bhs · 2017-09-12T19:44:14Z

opentracing-examples/README.md

@@ -0,0 +1,14 @@
+# OpenTracing-Java examples


We need a "Why does this exist" sort of thing... Could just be ## Module purpose or whatever. My TL;DR is (1) something to test API changes against, (2) something to look at for common instrumentation patterns, (3) something to use for Tracer regression testing.

... oh, and let's make a pointer in the main OT-Java README, too.

should be pointer under "Development" section?

coveralls · 2017-09-13T03:19:48Z

Coverage remained the same at 74.074% when pulling ed08547 on malafeev:master into 431ebc3 on opentracing:master.

coveralls · 2017-09-14T03:42:39Z

Coverage remained the same at 74.074% when pulling 74da38a on malafeev:master into 431ebc3 on opentracing:master.

pavolloffay · 2017-09-14T07:20:31Z

...examples/src/test/java/io/opentracing/examples/activate_deactivate/TestScheduledActions.java

+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.junit.Assert.assertEquals;
+
+public class TestScheduledActions {


Could you please rename test classes to align with naming convention used in this repo. All other test files are named WhateverTest

coveralls · 2017-09-14T07:36:52Z

Coverage remained the same at 74.074% when pulling b4e547d on malafeev:master into 431ebc3 on opentracing:master.

tedsuo · 2017-09-14T16:53:43Z

@pavolloffay @malafeev is this good enough to merge at this point? There are people waiting to add more test cases. If it's just style tweaks and documentation left to work on, can we merge and do that as subsequent PRs?

malafeev · 2017-09-15T06:59:37Z

I think it could be merged although more explanations should be added to examples.

pavolloffay · 2017-09-15T16:03:23Z

Yes, it looks better and cleaner, however, I still think it needs more comments. let's not block others and merge this.

add OpenTracing-Examples

d20f546

add README

744ebd8

tylerbenson approved these changes Sep 9, 2017

View reviewed changes

Merge branch 'master' of https://github.com/opentracing/opentracing-java

2fc00b0

pavolloffay reviewed Sep 11, 2017

View reviewed changes

apply feedback

cce6752

pavolloffay reviewed Sep 12, 2017

View reviewed changes

bhs reviewed Sep 12, 2017

View reviewed changes

more improvements

ed08547

rename

74da38a

pavolloffay reviewed Sep 14, 2017

View reviewed changes

rename test classes

b4e547d

tylerbenson mentioned this pull request Sep 15, 2017

Add examples for async as test cases #183

Closed

tedsuo merged commit 767f716 into opentracing:master Sep 15, 2017


		Examples of common instrumentation patterns:

		- activate_deactivate - callbacks finish at some time.

add OpenTracing-Examples #174

add OpenTracing-Examples #174

Conversation

malafeev commented Sep 5, 2017

coveralls commented Sep 5, 2017

yurishkuro commented Sep 5, 2017

coveralls commented Sep 5, 2017

objectiser commented Sep 5, 2017

malafeev commented Sep 7, 2017

carlosalberto commented Sep 7, 2017

tedsuo commented Sep 8, 2017

pavolloffay 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

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

coveralls commented Sep 12, 2017

coveralls commented Sep 12, 2017

pavolloffay commented Sep 12, 2017

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

bhs 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

coveralls commented Sep 13, 2017

coveralls commented Sep 14, 2017

Choose a reason for hiding this comment

coveralls commented Sep 14, 2017

tedsuo commented Sep 14, 2017

malafeev commented Sep 15, 2017

pavolloffay commented Sep 15, 2017