Skip to content

Commit

Permalink
Adds ExtraFieldPropagation.Customizer to implement secondary sampling
Browse files Browse the repository at this point in the history
Currently, we have a test `SecondarySampling` which is complex to
implement as it assumes conditional decisions will result in use of
different reporting libraries. Ex normal zipkin for sampled data, and
a different format for secondary sinks.

After a chat with Nara, we realized that data routing logic is cheapest
to address outside the process. In other words, if a secondary sampling
decision says "record" but only for a part of the network, this data
for that subtree will still end up posting to the same place.

With this in mind, secondary sampling can be far simpler, driven mainly
by decisions made on "extra fields". This change adds two things to
allow this to happen.

* `ExtraFieldPropagation.Customizer` which allows you to set a secondary decision (ex via sampledLocal)
* `Tracing.Builder.alwaysReportSpans()` which allows the normal span reporter to get everything recorded.

This PR neetds a little more work as the `SecondarySampling` test needs
to be redone to prove it makes things simpler.

Fixes #955
  • Loading branch information
Adrian Cole committed Aug 11, 2019
1 parent 4e6beda commit bf41390
Show file tree
Hide file tree
Showing 10 changed files with 404 additions and 68 deletions.
41 changes: 36 additions & 5 deletions brave/src/main/java/brave/Tracing.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import brave.internal.recorder.PendingSpans;
import brave.propagation.B3Propagation;
import brave.propagation.CurrentTraceContext;
import brave.propagation.ExtraFieldPropagation;
import brave.propagation.Propagation;
import brave.propagation.TraceContext;
import brave.sampler.Sampler;
Expand Down Expand Up @@ -142,7 +143,7 @@ public static final class Builder {
Clock clock;
Sampler sampler = Sampler.ALWAYS_SAMPLE;
CurrentTraceContext currentTraceContext = CurrentTraceContext.Default.inheritable();
boolean traceId128Bit = false, supportsJoin = true;
boolean traceId128Bit = false, supportsJoin = true, alwaysReportSpans = false;
Propagation.Factory propagationFactory = B3Propagation.FACTORY;
ErrorParser errorParser = new ErrorParser();
List<FinishedSpanHandler> finishedSpanHandlers = new ArrayList<>();
Expand Down Expand Up @@ -193,7 +194,7 @@ public Builder localPort(int localPort) {
}

/**
* Sets the {@link zipkin2.Span#localEndpoint Endpoint of the local service} being traced.
* Sets the {@link zipkin2.Span#localEndpoint() Endpoint of the local service} being traced.
*
* @deprecated Use {@link #localServiceName(String)} {@link #localIp(String)} and {@link
* #localPort(int)}. Will be removed in Brave v6.
Expand Down Expand Up @@ -323,13 +324,18 @@ public Builder errorParser(ErrorParser errorParser) {
* <h3>Advanced notes</h3>
*
* <p>This is named firehose as it can receive data even when spans are not sampled remotely.
* For example, {@link FinishedSpanHandler#alwaysSampleLocal()} will generate data for all traced
* requests while not affecting headers. This setting is often used for metrics aggregation.
* For example, {@link FinishedSpanHandler#alwaysSampleLocal()} will generate data for all
* traced requests while not affecting headers. This setting is often used for metrics
* aggregation.
*
*
* <p>Your handler can also be a custom span transport. When this is the case, set the {@link
* #spanReporter(Reporter) span reporter} to {@link Reporter#NOOP} to avoid redundant conversion
* overhead.
*
* @see #alwaysReportSpans()
* @see TraceContext#sampledLocal()
* @see ExtraFieldPropagation.FactoryBuilder#addCustomizer(ExtraFieldPropagation.Customizer)
*/
public Builder addFinishedSpanHandler(FinishedSpanHandler finishedSpanHandler) {
if (finishedSpanHandler == null) {
Expand All @@ -339,6 +345,31 @@ public Builder addFinishedSpanHandler(FinishedSpanHandler finishedSpanHandler) {
return this;
}

/**
* When true, all spans {@link TraceContext#sampledLocal() sampled locally} are reported to the
* {@link #spanReporter(Reporter) span reporter}, even if they aren't sampled remotely. Defaults
* to false.
*
* <p>The primary use case is to implement a sampling overlay, such as boosting the sample rate
* for a subset of the network depending on the value of an {@link ExtraFieldPropagation extra
* field}. This means that data will report when either the trace is normally sampled, or
* secondarily sampled via a custom header.
*
* <p>This is simpler than {@link #addFinishedSpanHandler(FinishedSpanHandler)}, because you
* don't have to duplicate transport mechanics already implemented in the {@link
* #spanReporter(Reporter) span reporter}. However, this assumes your backend can properly
* process the partial traces implied when using conditional sampling. For example, if your
* sampling condition is not consistent on a call tree, the resulting data could appear broken.
*
* @see #addFinishedSpanHandler(FinishedSpanHandler)
* @see TraceContext#sampledLocal()
* @see ExtraFieldPropagation.FactoryBuilder#addCustomizer(ExtraFieldPropagation.Customizer)
*/
public Builder alwaysReportSpans() {
this.alwaysReportSpans = true;
return this;
}

public Tracing build() {
if (clock == null) clock = Platform.get().clock();
if (localIp == null) localIp = Platform.get().linkLocalIp();
Expand Down Expand Up @@ -389,7 +420,7 @@ static final class Default extends Tracing {
FinishedSpanHandler zipkinFirehose = FinishedSpanHandler.NOOP;
if (builder.spanReporter != Reporter.NOOP) {
zipkinFirehose = new ZipkinFinishedSpanHandler(builder.spanReporter, errorParser,
builder.localServiceName, builder.localIp, builder.localPort);
builder.localServiceName, builder.localIp, builder.localPort, builder.alwaysReportSpans);
finishedSpanHandlers = new ArrayList<>(finishedSpanHandlers);
finishedSpanHandlers.add(zipkinFirehose);
}
Expand Down
2 changes: 1 addition & 1 deletion brave/src/main/java/brave/handler/FinishedSpanHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public abstract class FinishedSpanHandler {

/**
* When true, all spans become real spans even if they aren't sampled remotely. This allows
* firehose instances (such as metrics) to consider attributes that are not always visible
* finished span handlers (such as metrics) to consider attributes that are not always visible
* before-the-fact, such as http paths. Defaults to false and affects {@link
* TraceContext#sampledLocal()}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,17 @@
public final class ZipkinFinishedSpanHandler extends FinishedSpanHandler {
final Reporter<zipkin2.Span> spanReporter;
final MutableSpanConverter converter;
final boolean alwaysReportSpans;

public ZipkinFinishedSpanHandler(Reporter<zipkin2.Span> spanReporter,
ErrorParser errorParser, String serviceName, String ip, int port) {
ErrorParser errorParser, String serviceName, String ip, int port, boolean alwaysReportSpans) {
this.spanReporter = spanReporter;
this.converter = new MutableSpanConverter(errorParser, serviceName, ip, port);
this.alwaysReportSpans = alwaysReportSpans;
}

@Override public boolean handle(TraceContext context, MutableSpan span) {
if (!Boolean.TRUE.equals(context.sampled())) return true;
if (!alwaysReportSpans && !Boolean.TRUE.equals(context.sampled())) return true;

Span.Builder builderWithContextData = Span.newBuilder()
.traceId(context.traceIdString())
Expand Down
Loading

0 comments on commit bf41390

Please sign in to comment.