Sleuth is unlike most tracing configuration libraries, as it has a
legacy from 1.x as being primarily log correlation. In short, when
Zipkin is not installed, it ignored the sampling properties. This
behavior was managed implicitly through an untested combination of
configuration conventions between the core and zipkin modules.
16fb8e3 broke this and this change puts it back, in a way not tightly
coupled to Zipkin and neither requires the more simple, but confusing
"import SamplerAutoConfiguration" approach. It also backfills tests
that should have broken earlier.
In practice, a site that only uses logging is likely uniform in that, so
whether or not the sampled bit is set is of no consequence. However,
there's a chance that someone might rely on this historical behavior.
We should follow-up on 3.0 and remove this as it is very unintuitive to
intentionally ignore sampling properties. For now, this restores the old
behavior based on heuristics of bean definitions.
Before, spring-cloud-sleuth-zipkin had to import `SamplerAutoConfiguration`
directly to unwind a sampler ordering problem caused by `TraceAutoConfiguration`
defining the default `Sampler` bean.
This fixes that by moving the default `Sampler` to where it belongs
(`SamplerAutoConfiguration`) and having `TraceAutoConfiguration` import
the sampling configuration directly as opposed to relying on auto-configuration
ordering. Finally it removes the mistake of setting `SamplerAutoConfiguration`
as auto-configuration in the first place.
The name `SamplerAutoConfiguration` was left alone because changing it would
interfere with 3rd party code that formerly imported it to correct this issue
in their non-zipkin setups.
Fixes#1618
This tries to pick in the relevant changes made in 3813cf9dd4
Notably, there's one glitch, `TraceFeignAspect` still passes bean, not
wrappedBean. The latter trips out `ManuallyCreatedLoadBalancerFeignClientTests`
as the nonexistenturl raises a hard error in Ribbon. I *think* this is a bug and
tests need to just adjust for that, but need a second opinion. Also, I'm not
entirely sure the intent of using the broken url then asserting against a
success result..
In looking at underlying HttpClient mechanics, I noticed the reactor
call doesn't happen until subscribe. Before this change, we started the
client span at the ExchangeFilterFunction, not at subscribe time.
In many places, the trace context of callbacks was accidentally set to
the client span, not the invocation context. I noticed a hack trying to
work around this. This code fixes all the problems around context. It
also removes some sporadic logging, which was only applied to a few
hooks.
Finally, this adds Brave tests which would have caught the problems
earlier. Notably, there is still more work to do as this will not help
with duplicate instrumentation, which is normal when reactor-netty is
the WebClient's HTTP connector.
This consolidates access to the `HttpTracing` bean so that the overhead
of netty `HttpClient` is reduced. It also pulls the reactor-netty test
into its own file.
Probably due to code drift, `HookRegisteringBeanDefinitionRegistryPostProcessor.setupHooks`
was used as an object method, which led to a confusing no-op chain in
tests.
This ensures it is used consistently as a static method, and also
corrects a bean accidentally marked static for testing.
There are numerous places where code defensively guards access to
BeanFactory methods. This centralizes the code, first using in reactor
as that's the more performance sensitive.
This also weaves in feedback from #1541 given by @simonbasle and @robotmrv
Before, we used ConfigurableApplicationContext or BeanFactory eventhough
we already had a reference to ConfigurableApplicationContext. This uses
the latter consistently, avoiding a state condition that caused more
code.
This also corrects some misnamed tests and adjusts them to verify only
what they are responsible for.
