Introduces TraceSampler which replaces TraceFilter #118
Conversation
eb888a5 to
aa04e05
Compare
|
Validated the resulting pom has no zipkin-java dependency, and the classes added are <10K |
e2408d0 to
85ec72f
Compare
| @@ -37,7 +36,7 @@ public static Builder builder() { | |||
| abstract ClientSpanAndEndpoint spanAndEndpoint(); | |||
| abstract Random randomGenerator(); | |||
| abstract SpanCollector spanCollector(); | |||
| abstract List<TraceFilter> traceFilters(); | |||
There was a problem hiding this comment.
no more list.. if people have cause to create multiple tiers of filtering, they can make a composite one.
There was a problem hiding this comment.
Yes, agree. I don't think anyone actually used multiple filters.
85ec72f to
160f7a3
Compare
| <packaging>jar</packaging> | ||
| <name>brave-tracefilters</name> | ||
| <name>brave-tracesamplers</name> |
There was a problem hiding this comment.
ps kindof inclined to call this brave-zookeeper or brave-sampler-zookeeper
There was a problem hiding this comment.
because if we add something inside that doesn't have a ZK dep, then we'd end up with a similar problem as if we put it in core (the problem of unnecessary deps).
There was a problem hiding this comment.
Yes, that's a good point. Let's do it!
There was a problem hiding this comment.
TraceFilter worked off a span id, which isn't always the trace id. This is confusing and could lead to sparse traces. This eliminates use of TraceFilter, replaced with TraceSampler, which works off the trace id and is implemented by a benchmarked implementation in zipkin-java. The use of zipkin-java's TraceIdSampler is shaded, so as to neither introduce a dependency, nor the perils of copy/paste. Impact on users is that they should stop using TraceFilter immediately as Brave will ignore them. Most often, they will want to only provide a threshold, ex `TraceSampler.create(0.3f)` means retain 30%. We also no longer publish `brave-tracefilters` and instead an equivalent `brave-tracesamplers`, which has the same implementation, except based on trace id as opposed to span id. Finally, those using zookeeper should set the rate as a percentage, not a ratio. For example, `/brave/samplerate` would be 0.2 to retain 20%.
160f7a3 to
beac719
Compare
|
I think this is good to go. 👍 This is a breaking change so if we are strict about semantic versioning it should be 4.x. However I don't feel like it should be a major bump. Major bump would be rip out thrift and replace with Java implementation or replace propagation of state with new implementation. wdyt? |
|
I think we've already tainted 3.x with similar changes :) This implies that
the front number is more a marketing one.. I'd recommend we make 4.x count
and just increment minor, particularly as this is an edge-case feature.
|
|
Yes, agree :) Previous breaking changes were renames as far as I can remember (like scribe span collector). But still breaking changes. This one has a larger impact. Let's make it 4.x |
|
oh I actually meant save 4.x for larger impact (like opentracing) as this
is an ancillary function.
But if we do want to make this 4.x, then I'd just toss the classes instead
of deprecating. Which do you prefer?
|
|
Oh, ok, let's stick to my initial, and your view and leave it at to 3.x. We should just make the release notes clear about what needs to change. |
|
yeah. essentially...
|
|
👍 |
Introduces TraceSampler which replaces TraceFilter
TraceFilter worked off a span id, which isn't always the trace id. This
is confusing and could lead to sparse traces. This eliminates use of
TraceFilter, replaced with TraceSampler, which works off the trace id
and is implemented by a benchmarked implementation in zipkin-java.
The use of zipkin-java's TraceIdSampler is shaded, so as to neither
introduce a dependency, nor the perils of copy/paste.
Impact on users is that they should stop using TraceFilter immediately
as Brave will ignore them. Most often, they will want to only provide a
threshold, ex
TraceSampler.create(0.3f)means retain 30%.We also no longer publish
brave-tracefiltersand instead an equivalentbrave-tracesamplers, which has the same implementation, except basedon trace id as opposed to span id.
Finally, those using zookeeper should set the rate as a percentage, not
a ratio. For example,
/brave/sampleratewould be 0.2 to retain 20%.