Skip to content

Commit

Permalink
Reverts permittance of duplicate handlers (#1001)
Browse files Browse the repository at this point in the history
It is a worse bug to allow duplicate handlers than it is to allow
duplicate configuration. Duplicate handlers translate directly into
runtime overhead, particularly as there is special casing for one
handler.

Throwing an exception was suggested here #961, but that is not a good
choice as it would almost certainly lead to someone having to do an
emergency release to undo it. This changes to logging on dupe, so that
we don't propagate this problem into future work such as collector
handlers.

See openzipkin/zipkin#2807 (comment)
  • Loading branch information
adriancole authored Oct 2, 2019
1 parent 5f2d6d1 commit 75dab65
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 20 deletions.
27 changes: 12 additions & 15 deletions brave/src/main/java/brave/Tracing.java
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
import brave.sampler.SamplerFunction;
import java.io.Closeable;
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.Locale;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -146,13 +148,7 @@ public static final class Builder {
boolean traceId128Bit = false, supportsJoin = true, alwaysReportSpans = false;
Propagation.Factory propagationFactory = B3Propagation.FACTORY;
ErrorParser errorParser = new ErrorParser();
// Intentional dupes would be surprising. Be very careful when adding features here that no
// other list parameter extends FinishedSpanHandler. If it did, it could be accidentally added
// result in dupes here. Any component that provides a FinishedSpanHandler needs to be
// explicitly documented to not be added also here, to avoid dupes. That or mark a provider
// method protected, but still the user could accidentally create a dupe by misunderstanding and
// overriding public so they can add it here.
ArrayList<FinishedSpanHandler> finishedSpanHandlers = new ArrayList<>();
Set<FinishedSpanHandler> finishedSpanHandlers = new LinkedHashSet<>(); // dupes not ok

/**
* Lower-case label of the remote node in the service graph, such as "favstar". Avoid names with
Expand Down Expand Up @@ -339,15 +335,16 @@ public Builder errorParser(ErrorParser errorParser) {
* #spanReporter(Reporter) span reporter} to {@link Reporter#NOOP} to avoid redundant conversion
* overhead.
*
* @param handler skipped if {@link FinishedSpanHandler#NOOP} or already added
* @see #alwaysReportSpans()
* @see TraceContext#sampledLocal()
*/
public Builder addFinishedSpanHandler(FinishedSpanHandler finishedSpanHandler) {
if (finishedSpanHandler == null) {
throw new NullPointerException("finishedSpanHandler == null");
}
if (finishedSpanHandler != FinishedSpanHandler.NOOP) { // lenient on config bug
this.finishedSpanHandlers.add(finishedSpanHandler);
public Builder addFinishedSpanHandler(FinishedSpanHandler handler) {
if (handler == null) throw new NullPointerException("finishedSpanHandler == null");
if (handler != FinishedSpanHandler.NOOP) { // lenient on config bug
if (!finishedSpanHandlers.add(handler)) {
Platform.get().log("Please check configuration as %s was added twice", handler, null);
}
}
return this;
}
Expand Down Expand Up @@ -429,7 +426,7 @@ static final class Default extends Tracing {
FinishedSpanHandler finishedSpanHandler =
zipkinReportingFinishedSpanHandler(builder.finishedSpanHandlers, zipkinHandler, noop);

ArrayList<FinishedSpanHandler> orphanedSpanHandlers = new ArrayList<>();
Set<FinishedSpanHandler> orphanedSpanHandlers = new LinkedHashSet<>();
for (FinishedSpanHandler handler : builder.finishedSpanHandlers) {
if (handler.supportsOrphans()) orphanedSpanHandlers.add(handler);
}
Expand Down Expand Up @@ -509,7 +506,7 @@ private void maybeSetCurrent() {
}

static FinishedSpanHandler zipkinReportingFinishedSpanHandler(
ArrayList<FinishedSpanHandler> input, FinishedSpanHandler zipkinHandler, AtomicBoolean noop) {
Set<FinishedSpanHandler> input, FinishedSpanHandler zipkinHandler, AtomicBoolean noop) {
ArrayList<FinishedSpanHandler> defensiveCopy = new ArrayList<>(input);
// When present, the Zipkin handler is invoked after the user-supplied finished span handlers.
if (zipkinHandler != FinishedSpanHandler.NOOP) defensiveCopy.add(zipkinHandler);
Expand Down
10 changes: 5 additions & 5 deletions brave/src/test/java/brave/TracingTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ public class TracingTest {
}
}

/** This test lets future maintainers know that we don't dedupe, which can explain future bugs. */
@Test public void finishedSpanHandler_doesntDedupe() {
/** This test shows that duplicates are not allowed. This prevents needless overhead. */
@Test public void finishedSpanHandler_dupesIgnored() {
FinishedSpanHandler finishedSpanHandler = new FinishedSpanHandler() {
@Override public boolean handle(TraceContext context, MutableSpan span) {
return true;
Expand All @@ -142,12 +142,12 @@ public class TracingTest {

try (Tracing tracing = Tracing.newBuilder()
.addFinishedSpanHandler(finishedSpanHandler)
.addFinishedSpanHandler(finishedSpanHandler)
.addFinishedSpanHandler(finishedSpanHandler) // dupe
.build()) {
assertThat(tracing.tracer().finishedSpanHandler).extracting("handlers")
.satisfies(handlers -> assertThat((FinishedSpanHandler[]) handlers)
.startsWith(finishedSpanHandler, finishedSpanHandler)
.hasSize(3) // zipkin and the above
.startsWith(finishedSpanHandler)
.hasSize(2) // zipkin and the above
);
}
}
Expand Down

0 comments on commit 75dab65

Please sign in to comment.