openzipkin-contrib / brave-ratpack Goto Github PK

Migrated from hyleung/ratpack-zipkin#64

Ratpack 1.6 is out and should include a client interceptor API now. I suspect we will get better milage from using the interceptors and hope that it may also fix some anomalies around client tracing that have been seen under heavy load.

I already have a workaround to support RxJava Observable workloads (rather than using TracedParallelBatch Promises), but it relies on direct access to the TraceContextHolder class which is marked private. This appears to only work in Groovy.

Please add additional options for passing the trace context across threads, or allow users to manually set the trace context back after we switch threads so the Trace code can find it.

Example workaround:

def tracer = Tracing?.current()?.tracer()
TraceContext context = tracer.currentSpan()?.context()
dev action = Observable.just("test")

action.doOnSubscribe {
  Execution.current().add(new RatpackCurrentTraceContext.TraceContextHolder(context))

  tracer.currentSpan().tag("test key", "test value")  
}.subscribe()

Add .mailmap file to map commits to correct email address.

Because of how the http client is bound in the module, it is difficult for consuming applications to leverage the zipkin interceptors while specifying additional http spec properties like pool size, read timeout, connect timeout, etc. Refer to https://github.com/openzipkin-contrib/brave-ratpack/blob/master/src/main/java/ratpack/zipkin/ServerTracingModule.java#L61

Currently, the way to work around this is to build and inject a non Zipkin annoted client by using copyWith to create an entirely new client. For example (kotlin):

    @Provides
    @Singleton
    fun integrationHttpClient(
        properties: IntegrationProperties,
        @Zipkin httpClient: HttpClient,
        clientTracingInterceptor: ClientTracingInterceptor
    ): HttpClient {
        return httpClient.copyWith {
            it.poolSize(properties.poolSize)
            it.connectTimeout(Duration.ofMillis(properties.connectTimeoutMillis))
            it.readTimeout(Duration.ofMillis(properties.readTimeoutMillis))
            it.idleTimeout(Duration.ofMillis(properties.idleTimeoutMillis))
            it.maxContentLength(properties.maxContentLength)
            // adds compression headers if they don't exist, otherwise leaves existing values in the request
            it.requestIntercept { request ->
                request.headers.add(HttpHeaderNames.ACCEPT_ENCODING, COMPRESSION_VALUE)
                clientTracingInterceptor.request(request)
            }
            it.responseIntercept { clientTracingInterceptor.response(it) }
            it.errorIntercept { clientTracingInterceptor.error(it) }
        }
    }

Is there a nicer way to do this? Or is it a matter of documenting how to layer one's own configurations on top of the built-in zipkin client?

1. Could we require consumers to bind their own client, and then in this module layer the interceptors on top using copyWith?
2. Could we then take if further, where if consumer did not provide an http client at all, we build a new one and use that?

Right now this library has a dependency on ratpack and ratpack-guice. Ideally this should be split into separate modules so users have an easy way to opt out of the Guice libraries if they want.

ServerRequest and ServerResponse types are public, so I'm not sure how we should handle them. I'm not sure if they are public as a part of the api, or accidentally so.

I'd like to migrate this to latest brave, but adhering to these might make things a bit more difficult than if they never exists or were package private..

The HTTP client wrapper exhibits a context memory leak when using streaming requests. The effected code is likely here https://github.com/openzipkin-contrib/brave-ratpack/blob/master/src/main/java/ratpack/zipkin/internal/ZipkinHttpClientImpl.java#L129

This issue was originally reported by @Bijnagte and also reported that the memory leak went away if code was switched back to use a non streaming request.

The latest Brave library has some nice additions. Unfortunately there are some incompatible changes which require a bit more work than just revving the version.

TraceContextHolder Class has to be Singleton

Multiple instance of the same class is found in the registries of the Current Execution object.
This causes RatpackCurrentTraceContext.get() function to return null context on creation of new spans. Hence forming separate traces.

init function in TracingPropagationExecInitializer is trying to get HttpTracing.class from the execution object i pass but instead it should maybeGet(TraceContextHolder.class).
Since in my registry entries only this is present.
RegistryEntry{type=ratpack.zipkin.internal.RatpackCurrentTraceContext$TraceContextHolder, value=ratpack.zipkin.internal.RatpackCurrentTraceContext$TraceContextHolder@6139672b}.
Hence httpTracing is always null in init.

So init can't be used for manual setting of TraceContext on passing the current Execution because at that point RatpackCurrentTraceContext$TraceContextHolder will be having the CurrentTraceContext and not HttpTracing.class

Scope of the Span not working as expected with Ratpack Promises

newScope function in RatpackCurrentTraceContext has the return() which gets executed when scope of span closes when a promise is expected. But is scope is still not over.

This looks to be needed only for testing, so the scope should change to test.