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.

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

Fix empty context activation behaviour #3227

Merged
merged 2 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,11 @@ class ActiveStack {
*/
private final Deque<ElasticContext<?>> activeContextStack = new ArrayDeque<ElasticContext<?>>();

ActiveStack(int stackMaxDepth) {
private final EmptyElasticContext emptyContext;

ActiveStack(int stackMaxDepth, EmptyElasticContext emptyContextForTracer) {
this.stackMaxDepth = stackMaxDepth;
this.emptyContext = emptyContextForTracer;
}

@Nullable
Expand All @@ -80,7 +83,7 @@ public ElasticContext<?> currentContext() {
if (current instanceof ElasticContextWrapper) {
return ((ElasticContextWrapper<?>) current).getWrappedContext();
}
return current != null ? current : EmptyElasticContext.INSTANCE;
return current != null ? current : emptyContext;
}

boolean activate(ElasticContext<?> context, List<ActivationListener> activationListeners) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,12 @@ public class ElasticApmTracer implements Tracer {
private final Reporter reporter;
private final ObjectPoolFactory objectPoolFactory;

private final EmptyElasticContext emptyContext;

private final ThreadLocal<ActiveStack> activeStack = new ThreadLocal<ActiveStack>() {
@Override
protected ActiveStack initialValue() {
return new ActiveStack(transactionMaxSpans);
return new ActiveStack(transactionMaxSpans, emptyContext);
}
};

Expand Down Expand Up @@ -186,6 +188,7 @@ private static void checkClassloader() {

ElasticApmTracer(ConfigurationRegistry configurationRegistry, MetricRegistry metricRegistry, Reporter reporter, ObjectPoolFactory poolFactory,
ApmServerClient apmServerClient, final String ephemeralId, MetaDataFuture metaDataFuture) {
this.emptyContext = new EmptyElasticContext(this);
this.metricRegistry = metricRegistry;
this.configurationRegistry = configurationRegistry;
this.reporter = reporter;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,13 @@

import co.elastic.apm.agent.impl.transaction.AbstractSpan;
import co.elastic.apm.agent.impl.transaction.ElasticContext;
import co.elastic.apm.agent.tracer.Scope;

import javax.annotation.Nullable;

class EmptyElasticContext extends ElasticContext<EmptyElasticContext> {

static final ElasticContext<?> INSTANCE = new EmptyElasticContext();

private EmptyElasticContext() {
EmptyElasticContext(ElasticApmTracer tracer) {
super(tracer);
}

@Nullable
Expand All @@ -37,21 +35,6 @@ public AbstractSpan<?> getSpan() {
return null;
}

@Override
public EmptyElasticContext activate() {
return this;
}

@Override
public EmptyElasticContext deactivate() {
return this;
}

@Override
public Scope activateInScope() {
return NoopScope.INSTANCE;
}

@Override
public void incrementReferences() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
import co.elastic.apm.agent.sdk.logging.LoggerFactory;
import co.elastic.apm.agent.sdk.internal.util.LoggerUtils;
import co.elastic.apm.agent.tracer.Outcome;
import co.elastic.apm.agent.tracer.Scope;
import co.elastic.apm.agent.tracer.dispatch.BinaryHeaderGetter;
import co.elastic.apm.agent.tracer.dispatch.HeaderGetter;
import co.elastic.apm.agent.tracer.dispatch.TextHeaderGetter;
Expand All @@ -55,7 +54,6 @@ public abstract class AbstractSpan<T extends AbstractSpan<T>> extends ElasticCon
*/
protected final StringBuilder name = new StringBuilder();
protected final boolean collectBreakdownMetrics;
protected final ElasticApmTracer tracer;
protected final AtomicLong timestamp = new AtomicLong();
protected final AtomicLong endTimestamp = new AtomicLong();

Expand Down Expand Up @@ -197,7 +195,7 @@ public long getDuration() {
}

public AbstractSpan(ElasticApmTracer tracer) {
this.tracer = tracer;
super(tracer);
traceContext = TraceContext.with64BitId(this.tracer);
boolean selfTimeCollectionEnabled = !WildcardMatcher.isAnyMatch(tracer.getConfig(ReporterConfiguration.class).getDisableMetrics(), "span.self_time");
boolean breakdownMetricsEnabled = tracer.getConfig(CoreConfiguration.class).isBreakdownMetricsEnabled();
Expand Down Expand Up @@ -625,23 +623,6 @@ private boolean hasChildId(Id spanId) {
*/
public abstract Transaction getParentTransaction();

@Override
public T activate() {
tracer.activate(this);
return thiz();
}

@Override
public T deactivate() {
tracer.deactivate(this);
return thiz();
}

@Override
public Scope activateInScope() {
return tracer.activateInScope(this);
}

/**
* Set start timestamp
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package co.elastic.apm.agent.impl.transaction;

import co.elastic.apm.agent.impl.ElasticApmTracer;
import co.elastic.apm.agent.tracer.Scope;
import co.elastic.apm.agent.tracer.dispatch.BinaryHeaderSetter;
import co.elastic.apm.agent.tracer.dispatch.HeaderUtils;
import co.elastic.apm.agent.tracer.dispatch.TextHeaderGetter;
Expand All @@ -27,6 +29,12 @@

public abstract class ElasticContext<T extends ElasticContext<T>> implements co.elastic.apm.agent.tracer.ElasticContext<T> {

protected final ElasticApmTracer tracer;

protected ElasticContext(ElasticApmTracer tracer) {
this.tracer = tracer;
}

@Nullable
public abstract AbstractSpan<?> getSpan();

Expand All @@ -39,6 +47,25 @@ public final Transaction getTransaction() {
return contextSpan != null ? contextSpan.getParentTransaction() : null;
}

@Override
@SuppressWarnings("unchecked")
public T activate() {
tracer.activate(this);
return (T) this;
}

@Override
@SuppressWarnings("unchecked")
public T deactivate() {
tracer.deactivate(this);
return (T) this;
}

@Override
public Scope activateInScope() {
return tracer.activateInScope(this);
}

@Nullable
@Override
public co.elastic.apm.agent.impl.transaction.Span createSpan() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public class ElasticContextWrapper<T extends ElasticContext<T>> extends ElasticC
private final Map<Class<?>, ElasticContext<?>> contextWrappers;

public ElasticContextWrapper(int initialSize, ElasticContext<T> context) {
super(context.tracer);
this.contextWrappers = new HashMap<>(initialSize, 1.0f);
this.context = context;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,24 @@ void testActivateDeactivateTwice() {
transaction.end();
}

@Test
void testEmptyContextActivation() {
final Transaction transaction = startTestRootTransaction();
assertThat(tracerImpl.currentContext().getTransaction()).isNull();
tracerImpl.activate(transaction);
assertThat(tracerImpl.currentContext().getTransaction()).isEqualTo(transaction);

EmptyElasticContext empty = new EmptyElasticContext(tracerImpl);
empty.activate();
assertThat(tracerImpl.currentContext().getTransaction()).isNull();

empty.deactivate();
assertThat(tracerImpl.currentContext().getTransaction()).isEqualTo(transaction);
tracerImpl.deactivate(transaction);
assertThat(tracerImpl.currentContext().getTransaction()).isNull();
transaction.end();
}

@Test
void testOverrideServiceNameWithoutExplicitServiceName() {
final ElasticApmTracer tracer = new ElasticApmTracerBuilder()
Expand Down Expand Up @@ -714,19 +732,8 @@ void testUnknownConfiguration() {

private static final class TestContext extends ElasticContext<TestContext> {

@Override
public TestContext activate() {
return null;
}

@Override
public TestContext deactivate() {
return null;
}

@Override
public Scope activateInScope() {
return null;
private TestContext() {
super(null);
}

@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,8 @@ public class OTelBridgeContext extends ElasticContext<OTelBridgeContext> impleme
*/
private final Context otelContext;

private final ElasticApmTracer tracer;

private OTelBridgeContext(ElasticApmTracer tracer, Context otelContext) {
this.tracer = tracer;
super(tracer);
this.otelContext = otelContext;
}

Expand Down Expand Up @@ -99,23 +97,6 @@ public static OTelBridgeContext wrapElasticActiveSpan(ElasticApmTracer tracer, A
return new OTelBridgeContext(tracer, originalRootContext.with(otelSpan));
}

@Override
public OTelBridgeContext activate() {
tracer.activate(this);
return this;
}

@Override
public co.elastic.apm.agent.tracer.Scope activateInScope() {
return tracer.activateInScope(this);
}

@Override
public OTelBridgeContext deactivate() {
tracer.deactivate(this);
return this;
}

@Nullable
@Override
public AbstractSpan<?> getSpan() {
Expand Down
Loading