Skip to content

Commit

Permalink
Improved handling of Jaeger spans/scopes across threads (#3134)
Browse files Browse the repository at this point in the history
* Improved handling of Jaeger spans/scopes across threads. Fixes problems with MP async calls that resulted in memory leaks of ThreadLocalScope objects due to the failed attempt to close a scope from a different thread. New implementation uses a DataPropagationProvider to close a scope on the primary thread and re-open it on the secondary thread --and eventually close it there as well. Minor change to the DataPropagationProvider spi.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

* Deprecated rather than remove old clearData() method to ensure backward compatibility of the SPI.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>

* Fixed adding default to deprecated method.

Signed-off-by: Santiago Pericasgeertsen <santiago.pericasgeertsen@oracle.com>
  • Loading branch information
spericas authored Jun 22, 2021
1 parent 4ffadeb commit 576a666
Show file tree
Hide file tree
Showing 11 changed files with 320 additions and 22 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2020 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2019, 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -134,7 +134,7 @@ protected <T> Callable<T> wrap(Callable<T> task) {
PROVIDERS.forEach(provider -> provider.propagateData(properties.get(provider.getClass())));
return Contexts.runInContext(context.get(), task);
} finally {
PROVIDERS.forEach(DataPropagationProvider::clearData);
PROVIDERS.forEach(provider -> provider.clearData(properties.get(provider.getClass())));
}
};
} else {
Expand All @@ -153,7 +153,7 @@ protected Runnable wrap(Runnable command) {
PROVIDERS.forEach(provider -> provider.propagateData(properties.get(provider.getClass())));
Contexts.runInContext(context.get(), command);
} finally {
PROVIDERS.forEach(DataPropagationProvider::clearData);
PROVIDERS.forEach(provider -> provider.clearData(properties.get(provider.getClass())));
}
};
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Oracle and/or its affiliates.
* Copyright (c) 2020, 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,7 @@
package io.helidon.common.context.spi;

/**
* This is SPI provider which helps user to propagate values from one thread to another.
* This is an SPI provider which helps user to propagate values from one thread to another.
*
* Every provider has its method {@link #data()} invoked before thread switch, to obtain
* value for propagation. After the thread is switched, the new thread executes
Expand All @@ -41,8 +41,20 @@ public interface DataPropagationProvider<T> {
void propagateData(T data);

/**
* Clears the propagated date from the new thread when it finishes.
* Clears the propagated data from the new thread when it finishes. This
* method is deprecated in favor of {@link #clearData(Object)}.
*/
void clearData();
@Deprecated
default void clearData() {
}

/**
* Clears the propagated data from the new thread when it finishes.
*
* @param data data for propagation
*/
default void clearData(T data) {
clearData();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright (c) 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.helidon.common.context;

import io.helidon.common.context.spi.DataPropagationProvider;

import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.CoreMatchers.is;

/**
* Verifies backward compatibility of SPI after method deprecation.
*/
class DataPropagationProviderTest {

private boolean called = false;

class MyDataPropagationTest implements DataPropagationProvider<Object> {

@Override
public Object data() {
return null;
}

@Override
public void propagateData(Object data) {
}

/**
* Deprecated method should be called.
*/
@Override
public void clearData() {
called = true;
}
}

@Test
void testDeprecation() {
MyDataPropagationTest dpt = new MyDataPropagationTest();
dpt.clearData("foo"); // should call deprecated method
assertThat(called, is(true));
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Oracle and/or its affiliates.
* Copyright (c) 2020, 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -36,7 +36,7 @@ public void propagateData(Map<String, String> data) {
}

@Override
public void clearData() {
public void clearData(Map<String, String> data) {
JulMdc.clear();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020 Oracle and/or its affiliates.
* Copyright (c) 2020, 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -38,7 +38,7 @@ public void propagateData(Map<String, String> data) {
}

@Override
public void clearData() {
public void clearData(Map<String, String> data) {
ThreadContext.clearAll();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public void propagateData(Map<String, String> data) {
}

@Override
public void clearData() {
public void clearData(Map<String, String> data) {
MDC.clear();
}

Expand Down
4 changes: 4 additions & 0 deletions tracing/jaeger/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@
<groupId>io.helidon.common</groupId>
<artifactId>helidon-common</artifactId>
</dependency>
<dependency>
<groupId>io.helidon.common</groupId>
<artifactId>helidon-common-context</artifactId>
</dependency>
<dependency>
<groupId>io.helidon.config</groupId>
<artifactId>helidon-config</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright (c) 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.helidon.tracing.jaeger;

import io.helidon.common.context.Contexts;
import io.helidon.common.context.spi.DataPropagationProvider;

import io.opentracing.Scope;
import io.opentracing.Span;
import io.opentracing.Tracer;
import io.opentracing.util.GlobalTracer;

/**
* A data propagation provider for Jaeger. Makes sure span are properly propagated
* across threads managed by {@link io.helidon.common.context.ContextAwareExecutorService}.
*/
public class JaegerDataPropagationProvider implements DataPropagationProvider<JaegerDataPropagationProvider.JaegerContext> {

static class JaegerContext {
private final Span span;
private final Tracer tracer;
private Scope scope;

JaegerContext(Tracer tracer, Span span) {
this.tracer = tracer;
this.span = span;
}

Scope scope() {
return scope;
}
}

/**
* Closes scope in primary thread and returns a context to activate
* new scope in secondary thread.
*
* @return active span.
*/
@Override
public JaegerContext data() {
return Contexts.context().map(context -> {
context.get(Scope.class).ifPresent(Scope::close);
return context.get(Span.class).map(span -> {
Tracer tracer = context.get(Tracer.class).orElseGet(GlobalTracer::get);
return new JaegerContext(tracer, span);
}).orElse(null);
}).orElse(null);
}

/**
* Activates scope in secondary thread.
*
* @param context the context.
*/
@Override
public void propagateData(JaegerContext context) {
if (context != null) {
context.scope = context.tracer.scopeManager().activate(context.span);
}
}

/**
* Closes scope in secondary thread.
*/
@Override
public void clearData(JaegerContext context) {
if (context != null && context.scope != null) {
context.scope.close();
}
}
}
6 changes: 4 additions & 2 deletions tracing/jaeger/src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2019, 2020 Oracle and/or its affiliates.
* Copyright (c) 2019, 2021 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -21,6 +21,7 @@
requires io.helidon.common;
requires io.helidon.config;
requires io.helidon.tracing;
requires io.helidon.common.context;

requires java.logging;
requires io.opentracing.util;
Expand All @@ -30,8 +31,9 @@
// need to explicitly require transitive dependency, as jaeger is not a module
requires com.google.gson;


exports io.helidon.tracing.jaeger;

provides io.helidon.tracing.spi.TracerProvider with io.helidon.tracing.jaeger.JaegerTracerProvider;
provides io.helidon.common.context.spi.DataPropagationProvider with io.helidon.tracing.jaeger.JaegerDataPropagationProvider;

}
Loading

0 comments on commit 576a666

Please sign in to comment.