Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apache HTTP Client difference between Alfresco & Brave Zipkin #16

Open
todorinskiz opened this issue Mar 10, 2021 · 1 comment
Open
Assignees

Comments

@todorinskiz
Copy link
Member

The problematic method is located in eu.xenit.alfresco.instrumentation.repo.TracingHttpClient - a wrapper we use to instrument the Alfresco's org.apache.commons.httpclient.HttpClient for Solr.

    @Override
    public int executeMethod(HostConfiguration hostconfig, HttpMethod method, HttpState state) throws IOException {
        int responseStatus = 0;
        Span span = httpClientHandler.handleSend(injector, method);
        Throwable error = null;
        try (Tracer.SpanInScope ws = tracer.withSpanInScope(span)) {
            responseStatus = super.executeMethod(hostconfig, method, state);
        } catch (RuntimeException | Error e) {
            error = e;
            throw e;
        } finally {
            httpClientHandler.handleReceive(responseStatus, error, span);
        }
        return responseStatus;
    }

AS-IS brave.http.HttpClientHandler Java API:

    HttpClientHandler<HttpMethod, Integer> httpClientHandler = HttpClientHandler.create(httpTracing, new TracingHttpClientAdapter());


  /**
   * @since 4.3
   * @deprecated Since 5.7, use {@link #handleSend(HttpClientRequest)}, as this allows more advanced
   * samplers to be used.
   */
  @Deprecated public Span handleSend(Injector<Req> injector, Req request) {
    return handleSend(injector, request, request);
  }

  /**
   * @since 4.3
   * @deprecated since 5.12 use {@link #handleReceive(HttpClientResponse, Span)}
   */
  @Deprecated
  public void handleReceive(@Nullable Resp response, @Nullable Throwable error, Span span) {
    if (span == null) throw new NullPointerException("span == null");
    if (response == null && error == null) {
      throw new IllegalArgumentException(
        "Either the response or error parameters may be null, but not both");
    }

    if (response == null) {
      span.error(error).finish();
      return;
    }

    HttpClientResponse clientResponse;
    if (response instanceof HttpClientResponse) {
      clientResponse = (HttpClientResponse) response;
      if (clientResponse.error() == null && error != null) {
        span.error(error);
      }
    } else {
      clientResponse = new FromResponseAdapter<>(adapter, response, error);
    }
    handleFinish(clientResponse, span);
  }

TO-BE:

  HttpClientHandler<HttpClientRequest, HttpClientResponse> httpClientHandler = HttpClientHandler.create(httpTracing);

  /**
   * Like {@link #handleSend(HttpClientRequest)}, except explicitly controls the span representing
   * the request.
   *
   * @since 5.7
   */
  public Span handleSend(HttpClientRequest request, Span span) {
    if (request == null) throw new NullPointerException("request == null");
    if (span == null) throw new NullPointerException("span == null");
    defaultInjector.inject(span.context(), request);
    return handleStart(request, span);
  }


  /**
   * Finishes the client span after assigning it tags according to the response or error.
   *
   * <p>This is typically called once the response headers are received, and after the span is
   * {@link Tracer.SpanInScope#close() no longer in scope}.
   *
   * <p><em>Note</em>: It is valid to have a {@link HttpClientResponse} that only includes an
   * {@linkplain HttpClientResponse#error() error}. However, it is better to also include the
   * {@linkplain HttpClientResponse#request() request}.
   *
   * @see HttpResponseParser#parse(HttpResponse, TraceContext, SpanCustomizer)
   * @since 5.12
   */
  public void handleReceive(HttpClientResponse response, Span span) {
    handleFinish(response, span);
  }
@tgeens
Copy link
Member

tgeens commented Mar 15, 2021

@todorinskiz you are looking at the wrong dependency - you should be instrumenting org.apache.httpcomponents:httpclient, not org.apache.commons.httpcllient:commons-httpclient:3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants