openzipkin-contrib / brave-ratpack

Brave instrumentation for Ratpack
Apache License 2.0
5 stars 4 forks source link

Consumer applications not able to customize http client #30

Closed drmaas closed 5 years ago

drmaas commented 5 years ago

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?