Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds support for AutoFinishScope #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adds support for AutoFinishScope #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@
</developers>

<properties>
<maven.compiler.source>1.6</maven.compiler.source>
<maven.compiler.target>1.6</maven.compiler.target>
<maven.compiler.source>1.7</maven.compiler.source>
<maven.compiler.target>1.7</maven.compiler.target>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the java version bump?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try with resources in a test. I suppose that does not merit the change. I'll revert these.

<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<version.junit>4.12</version.junit>
Expand All @@ -48,6 +48,7 @@
<version.maven-javadoc-plugin>2.10.4</version.maven-javadoc-plugin>
<version.io.takari-maven>0.3.4</version.io.takari-maven>
<version.io.zikin.centralsync-maven-plugin>0.1.0</version.io.zikin.centralsync-maven-plugin>
<version.awaitility>3.0.0</version.awaitility>
</properties>

<dependencyManagement>
Expand Down Expand Up @@ -91,6 +92,12 @@
<version>${version.junit}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<version>${version.awaitility}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.opentracing</groupId>
<artifactId>opentracing-mock</artifactId>
Expand All @@ -99,7 +106,6 @@
<dependency>
<groupId>io.opentracing</groupId>
<artifactId>opentracing-util</artifactId>
<scope>test</scope>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make this provided and optional=true to avoid unnecessary transitive dependencies

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would result in java.lang.NoClassDefFoundError.

Copy link
Collaborator

@sjoerdtalsma sjoerdtalsma Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would result in java.lang.NoClassDefFoundError.

Currently, yes.
However, as I explained in #12 (comment), the util usage is entirely optional (based on an instanceof check, which can only happen if the application already added the util lib to the classpath).
Therefore, catching the NoClassDefFoundError and interpreting it as "probably isn't an instance of AutoFinishScopeManager" is valid logic in my opinion.

Libraries should not declare force transitive dependencies if they're optional.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be quite expensive to have it in runnable/callable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null)
       cont = ((AutoFinishScope) tracer.scopeManager().active()).capture();
    else
      cont = null;

Could be replaced by:

if (hasAutoFinishScopeManager(tracer) && tracer.scopeManager().active() != null) {
    cont = ((io.opentracing.util.AutoFinishScope) tracer.scopeManager().active()).capture();
}
...
private static boolean hasAutoFinishScopeManager(Tracer tracer) {
    try {
        return tracer.scopeManager() instanceof io.opentracing.util.AutoFinishScopeManager;
    } catch (NoClassDefFoundError utilityJarNotLoaded) {
        LOGGER.finest("The opentracing-util library is not available, therefore the ScopeManager cannot be the AutoFinishScopeManager.");
        return false;
    }
}

Just make sure not to import io.opentrating.util classes..

I agree this borders on being too far-fetched, just pointing out the possibilities.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better way would be to wrap all auto-finish related coded in its own (package protected) class and only access that if the hasAutoFinish.. test returned true.

Copy link
Collaborator

@sjoerdtalsma sjoerdtalsma Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That can be quite expensive to have it in runnable/callable.

Ok, then create a boolean constant and populate it in a static code block when the class is loaded..

For inspiration, you could look at PriorityComparator (optional use of javax.annotation.Priority) or TracerResolver (optional use of io.opentracing.util.GlobalTracer)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have consensus on the last proposal, static boolean. Then I don't mind adding that.

</dependency>
</dependencies>

Expand Down
16 changes: 15 additions & 1 deletion src/main/java/io/opentracing/contrib/concurrent/TracedCallable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,41 @@
import io.opentracing.Scope;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.util.AutoFinishScope;
import io.opentracing.util.AutoFinishScopeManager;

import java.util.concurrent.Callable;

/**
* @author Pavol Loffay
* @author Jose Montoya
*/
public class TracedCallable<V> implements Callable<V> {

private final Callable<V> delegate;
private final Span span;
private final Tracer tracer;
private final AutoFinishScope.Continuation cont;

public TracedCallable(Callable<V> delegate, Tracer tracer) {
this.delegate = delegate;
this.tracer = tracer;
this.span = tracer.activeSpan();

if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add braces in the if/else constructs here and following classes.

cont = ((AutoFinishScope) tracer.scopeManager().active()).capture();
else
cont = null;
}

@Override
public V call() throws Exception {
Scope scope = span == null ? null : tracer.scopeManager().activate(span, false);
Scope scope = null;
if (cont != null)
scope = cont.activate();
else if (span != null)
scope = tracer.scopeManager().activate(span, false);

try {
return delegate.call();
} finally {
Expand Down
18 changes: 16 additions & 2 deletions src/main/java/io/opentracing/contrib/concurrent/TracedRunnable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,40 @@
import io.opentracing.Scope;
import io.opentracing.Tracer;
import io.opentracing.Span;
import io.opentracing.util.AutoFinishScope;
import io.opentracing.util.AutoFinishScopeManager;

/**
* @author Pavol Loffay
* @author Jose Montoya
*/
public class TracedRunnable implements Runnable {

private final Runnable delegate;
private final Span span;
private final Tracer tracer;
private final AutoFinishScope.Continuation cont;

public TracedRunnable(Runnable delegate, Tracer tracer) {
this.delegate = delegate;
this.tracer = tracer;
this.span = tracer.activeSpan();

if (tracer.scopeManager() instanceof AutoFinishScopeManager && tracer.scopeManager().active() != null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation problem with this if/else construct.

cont = ((AutoFinishScope) tracer.scopeManager().active()).capture();
else
cont = null;
}

@Override
public void run() {
Scope scope = span == null ? null : tracer.scopeManager().activate(span, false);
try {
Scope scope = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation problem with this code block.

if (cont != null)
scope = cont.activate();
else if (span != null)
scope = tracer.scopeManager().activate(span, false);

try {
delegate.run();
} finally {
if (scope != null) {
Expand Down
121 changes: 121 additions & 0 deletions src/test/java/io/opentracing/contrib/concurrent/TracedAutoFinishTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
package io.opentracing.contrib.concurrent;

import io.opentracing.Scope;
import io.opentracing.mock.MockTracer;
import io.opentracing.util.AutoFinishScopeManager;
import org.junit.Before;
import org.junit.Test;

import java.util.concurrent.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand the imports.


import static org.awaitility.Awaitility.await;
import static org.hamcrest.core.IsEqual.equalTo;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;

/**
* @author Jose Montoya
*/
public class TracedAutoFinishTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should derive from the AbstractConcurrentTest and make use TestRunnable/Callable and latch mechanism, rather than use awaitility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was going to, but I needed a Thread.sleep within the runnable/callable to make sure that I could check the current thread's span and scope were close before the runnable returned. I didn't want to modify the existing TestRunnable, even though the original tests should still pass. If you're OK with me making those changes then I'll go ahead and do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could introduce a semaphore in the abstract test runnable/callable impls to allow the test to block their execution.

private static final int NUMBER_OF_THREADS = 4;
protected MockTracer mockTracer = new MockTracer(new AutoFinishScopeManager());

@Before
public void before() {
mockTracer.reset();
}

@Test
public void autoFinishScopeExecuteTest() throws InterruptedException {
Executor executor = new TracedExecutor(Executors.newFixedThreadPool(10), mockTracer);

try (Scope scope = mockTracer.buildSpan("auto-finish").startActive(true)){
executor.execute(new TestRunnable());
}

assertNull(mockTracer.scopeManager().active());
assertNull(mockTracer.activeSpan());
assertEquals(0, mockTracer.finishedSpans().size());
await().atMost(15, TimeUnit.SECONDS).until(finishedSpansSize(mockTracer), equalTo(1));
assertEquals(1, mockTracer.finishedSpans().size());
}


@Test
public void autoFinishScopeSubmitCallableTest() throws InterruptedException {
ExecutorService executorService =
new TracedExecutorService(Executors.newFixedThreadPool(NUMBER_OF_THREADS), mockTracer);

try (Scope scope = mockTracer.buildSpan("auto-finish").startActive(true)) {
executorService.submit(new TestCallable());
}

assertNull(mockTracer.scopeManager().active());
assertNull(mockTracer.activeSpan());
assertEquals(0, mockTracer.finishedSpans().size());
await().atMost(15, TimeUnit.SECONDS).until(finishedSpansSize(mockTracer), equalTo(1));
assertEquals(1, mockTracer.finishedSpans().size());
}

@Test
public void autoFinishScopeScheduleAtFixedRateTest() throws InterruptedException {
final CountDownLatch countDownLatch = new CountDownLatch(2);

ScheduledExecutorService executorService =
new TracedScheduledExecutorService(Executors.newScheduledThreadPool(NUMBER_OF_THREADS), mockTracer);

try (Scope scope = mockTracer.buildSpan("auto-finish").startActive(true)){
executorService.scheduleAtFixedRate(new Runnable() {
@Override
public void run() {
try {
Thread.sleep(300);
countDownLatch.countDown();
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}, 0, 200, TimeUnit.MILLISECONDS);
}

assertNull(mockTracer.scopeManager().active());
assertNull(mockTracer.activeSpan());
assertEquals(0, mockTracer.finishedSpans().size());
countDownLatch.await();
await().atMost(15, TimeUnit.SECONDS).until(finishedSpansSize(mockTracer), equalTo(1));
executorService.shutdown();
assertEquals(1, mockTracer.finishedSpans().size());
}

public static Callable<Integer> finishedSpansSize(final MockTracer tracer) {
return new Callable<Integer>() {
@Override
public Integer call() throws Exception {
return tracer.finishedSpans().size();
}
};
}

class TestRunnable implements Runnable {
@Override
public void run() {
try {
Thread.sleep(300);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
}

class TestCallable implements Callable<Void> {
@Override
public Void call() throws Exception {
try {
Thread.sleep(300);
} catch (InterruptedException e) {
e.printStackTrace();
}
return null;
}
}
}