Skip to content

Commit

Permalink
removes complexity and ordering problems by deconflating ExtraFieldPl…
Browse files Browse the repository at this point in the history
…ugin with FinishedSpanHandler
  • Loading branch information
Adrian Cole committed Aug 31, 2019
1 parent e95de8e commit b9eb0aa
Show file tree
Hide file tree
Showing 7 changed files with 32 additions and 104 deletions.
15 changes: 4 additions & 11 deletions brave/src/main/java/brave/Tracing.java
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ public Builder addFinishedSpanHandler(FinishedSpanHandler finishedSpanHandler) {
* @see #addFinishedSpanHandler(FinishedSpanHandler)
* @see TraceContext#sampledLocal()
* @see ExtraFieldPropagation.FactoryBuilder#addPlugin(ExtraFieldPlugin)
* @since 5.7
*/
public Builder alwaysReportSpans() {
this.alwaysReportSpans = true;
Expand Down Expand Up @@ -428,24 +429,16 @@ static final class Default extends Tracing {
builder.localServiceName, builder.localIp, builder.localPort, builder.alwaysReportSpans)
: FinishedSpanHandler.NOOP;

ArrayList<FinishedSpanHandler> finishedSpanHandlers =
new ArrayList<>(builder.finishedSpanHandlers);
// Register any extra field plugins as finished span handlers.
if (propagationFactory instanceof ExtraFieldPropagation.Factory) {
ExtraFieldPropagation.Factory extra = ((ExtraFieldPropagation.Factory) propagationFactory);
InternalPropagation.instance.extractFinishedSpanHandlers(extra, finishedSpanHandlers);
}

FinishedSpanHandler finishedSpanHandler =
zipkinReportingFinishedSpanHandler(finishedSpanHandlers, zipkinHandler, noop);
zipkinReportingFinishedSpanHandler(builder.finishedSpanHandlers, zipkinHandler, noop);

ArrayList<FinishedSpanHandler> orphanedSpanHandlers = new ArrayList<>();
for (FinishedSpanHandler handler : finishedSpanHandlers) {
for (FinishedSpanHandler handler : builder.finishedSpanHandlers) {
if (handler.supportsOrphans()) orphanedSpanHandlers.add(handler);
}

FinishedSpanHandler orphanedSpanHandler = finishedSpanHandler;
boolean allHandlersSupportOrphans = finishedSpanHandlers.equals(orphanedSpanHandlers);
boolean allHandlersSupportOrphans = builder.finishedSpanHandlers.equals(orphanedSpanHandlers);
if (!allHandlersSupportOrphans) {
orphanedSpanHandler =
zipkinReportingFinishedSpanHandler(orphanedSpanHandlers, zipkinHandler, noop);
Expand Down
6 changes: 0 additions & 6 deletions brave/src/main/java/brave/internal/InternalPropagation.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

import brave.ScopedSpan;
import brave.Span;
import brave.handler.FinishedSpanHandler;
import brave.propagation.ExtraFieldPropagation;
import brave.propagation.SamplingFlags;
import brave.propagation.TraceContext;
import java.util.List;
Expand Down Expand Up @@ -53,10 +51,6 @@ public static int sampled(boolean sampled, int flags) {
return flags;
}

/** Adds any finished span handlers used for extra fields into the given list. */
public abstract void extractFinishedSpanHandlers(
ExtraFieldPropagation.Factory factory, List<FinishedSpanHandler> out);

/**
* @param localRootId must be non-zero prior to instantiating {@link Span} or {@link ScopedSpan}
*/
Expand Down
27 changes: 4 additions & 23 deletions brave/src/main/java/brave/propagation/ExtraFieldPlugin.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
* Propagation plugins are self-contained and support advanced integration patterns such as metrics
* aggregation or sampling overlays.
*/
public abstract class ExtraFieldPlugin implements Comparable<ExtraFieldPlugin> {
public abstract class ExtraFieldPlugin {
/**
* Returns a list of unique lower-case field names used by this plugin.
*
Expand All @@ -36,10 +36,6 @@ public abstract class ExtraFieldPlugin implements Comparable<ExtraFieldPlugin> {
*/
protected abstract List<String> fieldNames();

protected int order() {
return 0;
}

/**
* This is called once during {@link TraceContext.Extractor#extract(Object)}, allowing you to
* decorate the primary trace state with secondary data from extra fields.
Expand All @@ -50,9 +46,9 @@ protected int order() {
* headers. Otherwise, you can set {@link TraceContextOrSamplingFlags.Builder#sampledLocal()} to
* make an overlaid decision.
*
* <p>If you are making an overlaid decision, you should either implement {@link
* #finishedSpanHandler()} here, or set {@link Tracing.Builder#alwaysReportSpans()} if your
* backend can tolerate inconsistent data.
* <p>If you are making an overlaid decision, you should either also implement a {@link
* FinishedSpanHandler}, or set {@link Tracing.Builder#alwaysReportSpans()} if yourbackend can
* tolerate inconsistent data.
*
* <p>The resulting field updater will called for each {@link #fieldNames() field name} in the
* order they were configured, not in the order headers were received.
Expand All @@ -71,12 +67,6 @@ protected FieldUpdater injectFieldUpdater(TraceContext context) {
return FieldUpdater.NOOP;
}

/** This allows you to set tags based on extra fields, most commonly for log correlation. */
// Intentionally protected to prevent accidental registration with Tracing.Builder
protected FinishedSpanHandler finishedSpanHandler() {
return FinishedSpanHandler.NOOP;
}

@Override public String toString() {
return getClass().getSimpleName() + "{" + fieldNames() + "}";
}
Expand Down Expand Up @@ -117,10 +107,6 @@ protected FieldUpdater extractFieldUpdater(TraceContextOrSamplingFlags.Builder b
return compositeFieldUpdater(fieldUpdaters, j);
}

@Override protected FinishedSpanHandler finishedSpanHandler() {
throw new AssertionError(); // expect special-casing as this is an internal class
}

@Override public String toString() {
return Arrays.toString(plugins);
}
Expand Down Expand Up @@ -149,9 +135,4 @@ FieldUpdater[] fieldUpdatersArray() {
return (FieldUpdater[]) fieldUpdatersArray;
}
}

@Override public int compareTo(ExtraFieldPlugin that) { // for Java 6+ api compat
int x = this.order(), y = that.order();
return (x < y) ? -1 : ((x == y) ? 0 : 1);
}
}
19 changes: 0 additions & 19 deletions brave/src/main/java/brave/propagation/ExtraFieldPropagation.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package brave.propagation;

import brave.Tracing;
import brave.handler.FinishedSpanHandler;
import brave.handler.MutableSpan;
import brave.internal.Nullable;
import brave.internal.PredefinedPropagationFields;
Expand Down Expand Up @@ -125,22 +124,6 @@ public interface FieldUpdater {
@Nullable String update(String name, @Nullable String value);
}

/** Sneaky accessibility trick. This allows us to access the finished span handler internally. */
static void extractFinishedSpanHandlers(Factory factory, List<FinishedSpanHandler> out) {
if (factory.plugin == null) return;
if (factory.plugin instanceof ExtraFieldPlugin.Multiple) {
for (ExtraFieldPlugin plugin : ((ExtraFieldPlugin.Multiple) factory.plugin).plugins) {
if (plugin.finishedSpanHandler() != FinishedSpanHandler.NOOP) {
out.add(plugin.finishedSpanHandler());
}
}
return;
}
if (factory.plugin.finishedSpanHandler() != FinishedSpanHandler.NOOP) {
out.add(factory.plugin.finishedSpanHandler());
}
}

static final class RedactOnInject extends ExtraFieldPlugin implements FieldUpdater {
final Set<String> redactedFields;

Expand Down Expand Up @@ -247,8 +230,6 @@ public FactoryBuilder addPrefixedFields(String prefix, Collection<String> fieldN

public Factory build() {
List<ExtraFieldPlugin> plugins = new ArrayList<>(this.plugins);
Collections.sort(plugins); // ensures fields are processed in the correct order

Set<String> distinctFields = new LinkedHashSet<>(fieldNames);
for (ExtraFieldPlugin plugin : plugins) {
distinctFields.addAll(plugin.fieldNames());
Expand Down
7 changes: 0 additions & 7 deletions brave/src/main/java/brave/propagation/SamplingFlags.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,8 @@
*/
package brave.propagation;

import brave.handler.FinishedSpanHandler;
import brave.internal.InternalPropagation;
import brave.internal.Nullable;
import java.util.ArrayList;
import java.util.List;

import static brave.internal.InternalPropagation.FLAG_DEBUG;
Expand All @@ -37,11 +35,6 @@ public class SamplingFlags {
return flags.flags;
}

@Override public void extractFinishedSpanHandlers(
ExtraFieldPropagation.Factory factory, List<FinishedSpanHandler> out) {
ExtraFieldPropagation.extractFinishedSpanHandlers(factory, out);
}

@Override
public TraceContext newTraceContext(int flags, long traceIdHigh, long traceId,
long localRootId, long parentId, long spanId, List<Object> extra) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
*/
package brave.features.propagation;

import brave.handler.FinishedSpanHandler;
import brave.handler.MutableSpan;
import brave.propagation.ExtraFieldPlugin;
import brave.propagation.ExtraFieldPropagation;
Expand All @@ -33,14 +32,6 @@ static ExtraFieldPlugin get() {
return Collections.singletonList("systems");
}

/**
* The systems field must be processed last as there may be multiple plugins that need to
* collaborate on the value.
*/
@Override protected int order() {
return Integer.MAX_VALUE;
}

/**
* Regardless of the primary sampling decision, this will sample when {@link #sample(boolean)} is
* called or {@link #isSampled(String, String)} evaluates to true.
Expand Down Expand Up @@ -95,17 +86,21 @@ boolean isSampled(String name, String value) {
}

/** This adds a comma-delimited tag named "systems" including "zipkin" if sampled remotely. */
@Override protected FinishedSpanHandler finishedSpanHandler() {
return new FinishedSpanHandler() {
@Override public boolean handle(TraceContext context, MutableSpan span) {
String systems = ExtraFieldPropagation.get(context, "systems");
if (Boolean.TRUE.equals(context.sampled())) {
systems = systems != null ? "zipkin," + systems : "zipkin";
}
if (systems != null) span.tag("systems", systems);
return true;
static class FinishedSpanHandler extends brave.handler.FinishedSpanHandler {
private static final FinishedSpanHandler DEFAULT = new FinishedSpanHandler();

static brave.handler.FinishedSpanHandler get() {
return DEFAULT;
}

@Override public boolean handle(TraceContext context, MutableSpan span) {
String systems = ExtraFieldPropagation.get(context, "systems");
if (Boolean.TRUE.equals(context.sampled())) {
systems = systems != null ? "zipkin," + systems : "zipkin";
}
};
if (systems != null) span.tag("systems", systems);
return true;
}
}

private SamplingSystemsPlugin() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
import brave.Tracing;
import brave.TracingCustomizer;
import brave.features.propagation.TriageSamplingPlugin.SetMultimap;
import brave.handler.FinishedSpanHandler;
import brave.handler.MutableSpan;
import brave.propagation.B3SinglePropagation;
import brave.propagation.ExtraFieldCustomizer;
import brave.propagation.ExtraFieldPlugin;
Expand Down Expand Up @@ -131,8 +129,9 @@ public class SecondarySamplingTest {
builder.addPlugin(SamplingSystemsPlugin.get());
};
Function<String, ExtraFieldCustomizer> configureAllSampling = localServiceName -> builder -> {
configureEdgeSampling.apply(localServiceName).customize(builder);
configureTriageSampling.apply(localServiceName).customize(builder);
builder.addPlugin(new EdgeSamplingPlugin(localServiceName));
builder.addPlugin(new TriageSamplingPlugin(triageKeyToService, localServiceName));
builder.addPlugin(SamplingSystemsPlugin.get());
};

Reporter<zipkin2.Span> traceForwarder = span -> {
Expand All @@ -150,7 +149,7 @@ public class SecondarySamplingTest {
TracedNode gateway;

@Test public void baseCase_nothingToZipkinWhenB3Unsampled() {
gateway = TracedNode.createServiceGraph(tracingFunction(addSampledZipkinTag));
gateway = TracedNode.createServiceGraph(tracingFunction(TracingCustomizer.NOOP));
Map<String, String> initialContext = new LinkedHashMap<>();
initialContext.put("b3", "0");
gateway.execute("/recommend", initialContext);
Expand All @@ -162,7 +161,7 @@ public class SecondarySamplingTest {
}

@Test public void baseCase_reportsToZipkinWhenB3Sampled() {
gateway = TracedNode.createServiceGraph(tracingFunction(addSampledZipkinTag));
gateway = TracedNode.createServiceGraph(tracingFunction(TracingCustomizer.NOOP));
Map<String, String> initialContext = new LinkedHashMap<>();
initialContext.put("b3", "1");
gateway.execute("/recommend", initialContext);
Expand All @@ -175,7 +174,7 @@ public class SecondarySamplingTest {
}

@Test public void baseCase_routingToRecommendations() throws Exception { // sanity check
gateway = TracedNode.createServiceGraph(tracingFunction(addSampledZipkinTag));
gateway = TracedNode.createServiceGraph(tracingFunction(TracingCustomizer.NOOP));
Map<String, String> initialContext = new LinkedHashMap<>();
initialContext.put("b3", "1");
gateway.execute("/recommend", initialContext);
Expand All @@ -186,7 +185,7 @@ public class SecondarySamplingTest {
}

@Test public void baseCase_routingToPlayback() throws Exception { // sanity check
gateway = TracedNode.createServiceGraph(tracingFunction(addSampledZipkinTag));
gateway = TracedNode.createServiceGraph(tracingFunction(TracingCustomizer.NOOP));
Map<String, String> initialContext = new LinkedHashMap<>();
initialContext.put("b3", "1");
gateway.execute("/play", initialContext);
Expand All @@ -206,17 +205,8 @@ public class SecondarySamplingTest {
);
}

TracingCustomizer addSampledZipkinTag = builder -> {
builder.addFinishedSpanHandler(new FinishedSpanHandler() {
@Override public boolean handle(TraceContext context, MutableSpan span) {
span.tag("systems", "zipkin");
return true;
}
});
};

@Test public void baseCase_b3_unsampled() { // sanity check
gateway = TracedNode.createServiceGraph(tracingFunction(addSampledZipkinTag));
gateway = TracedNode.createServiceGraph(tracingFunction(TracingCustomizer.NOOP));
Map<String, String> initialContext = new LinkedHashMap<>();
initialContext.put("b3", "0");
gateway.execute("/recommend", initialContext);
Expand Down Expand Up @@ -353,7 +343,8 @@ Function<String, Tracing> tracingFunction(TracingCustomizer customizer) {
return (localServiceName) -> {
Tracing.Builder result = Tracing.newBuilder()
.localServiceName(localServiceName)
.spanReporter(traceForwarder);
.spanReporter(traceForwarder)
.addFinishedSpanHandler(SamplingSystemsPlugin.FinishedSpanHandler.get());
customizer.customize(result);
return result.build();
};
Expand Down

0 comments on commit b9eb0aa

Please sign in to comment.