nike-inc / wingtips Goto Github PK

Users might want to fully control the log message output of distributed traces. To allow for this, SpanLoggingRepresentation should be refactored to an interface (with the existing JSON and KEY_VALUE enums continuing as available options people can use). Then Tracer.completeAndLogSpan() should be refactored to delegate the log message to the interface so that users can fully define the log message and format themselves if desired.

One possible solution:

• Rename SpanLoggingRepresentation into an interface named SpanLoggingStrategy.
• The SpanLoggingStrategy interface should have a single method: SpanLoggingStrategy.logCompletedSpan(Span completedSpan, Logger loggerToUse, String infoTag).
• Tracer.completeAndLogSpan() should delegate to whatever SpanLoggingStrategy is registered with it by replacing the loggerToUse.info(...) line with the new SpanLoggingStrategy method call.
• Migrate the existing JSON and KEY_VALUE SpanLoggingRepresentation enum values to a different enum named DefaultSpanLoggingStrategyOptions that implements SpanLoggingStrategy and results in the same log messages we see with the current code.

I would like to be able to use apache commons like this:
IOUtils.closeQuietly(span);
or use guava like this:
Closeables.close(span, true);

Neither is possible because Span implements AutoCloseable instead of Closeable. The naming here is confusing. Closeable extends AutoCloseable. Obviously, you can't pass the super class to a method that wants the subclass.

As discussed on openzipkin/b3-propagation#21 and first implemented here: https://github.com/openzipkin/brave/blob/master/brave/src/main/java/brave/propagation/B3SingleFormat.java https://github.com/openzipkin/brave/blob/master/brave/src/test/java/brave/propagation/B3SingleFormatTest.java

Let's support at least reading "b3" header from a single string, most commonly traceid-spanid-1
It would also be nice to support optionally writing this, especially in message providers or others with constrained environments.

Brave currently has a property like this, but its name could change with feedback:

    /**
     * When true, only writes a single {@link B3SingleFormat b3 header} for outbound propagation.
     *
     * <p>Use this to reduce overhead. Note: normal {@link Tracing#propagation()} is used to parse
     * incoming headers. The implementation must be able to read "b3" headers.
     */
    public Builder b3SingleFormat(boolean b3SingleFormat) {
      this.b3SingleFormat = b3SingleFormat;
      return this;
}

I think that adding support for Zipkin's error annotation will be a good addition to Wingtips.

There could be a method like Tracer.getInstance().completeRequestSpanWithError(); and something similar for sub-spans Tracer.getInstance().completeSubSpanWithError();

Investigate what it would take to become an implementation for OpenTracing: http://opentracing.io/

See the following link for context on this issue: openzipkin/b3-propagation#4

Also - maybe have the response header be Wingtips-RequestId rather than X-Wingtips-Request-Id to conform to https://tools.ietf.org/html/rfc6648?

The Dapper paper and many distributed tracing impls have support for arbitrary annotations/tags/baggage. Wingtips should follow suit to give users more flexibility in what wingtips does for them, compatibility with other impls, etc.

This may tie in with OpenTracing support.

Hi. I'm trying to get a webcallback to run inside a Runnable from an executorService.

I have a javax.enterprise.concurrent.ManagedExecutorService which is injected by CDI in Jboss by using the @Resource annotation.

Later I call...
this.executorService.execute(new WebClient(payload, attempts)); the WebClient.run() method is executed, but the WingtipsHttpClientBuilder instance used inside the WebClient run method appears to have no span context.

So I add withTracing() to my executor call.
this.executorService.execute(withTracing(() -> new WebClient(payload, attempts)));
I can see in my debugger that the WebClient is instantiated, but my breakpoint in the run method is never hit.

Can someone please give me some ideas on how to debug this? Can I provide some more information to help test this?

Many thanks in advance.

Wingtips is already towards zipkin compatibility via use of the B3 headers. These ensure that trace data propagated in-band with requests would carry forward even if the next hop is golang or ruby.

The next steps is to ensure the out-of-band data can be sent to zipkin, so that those using wing-tips and the zipkin ui can actually see the timing data not sent in the B3 headers.

For example, this could be reporting via POST to a zipkin http server.

Once this is done, it would be great to share the news by updating http://zipkin.io/pages/existing_instrumentations.html with wingtips. This could be done via a pull request updating this file: https://github.com/openzipkin/openzipkin.github.io/blob/master/_data/existing_instrumentations.yml

Assume that I have a Spring WebClient available instrumented with WingtipsSpringWebfluxExchangeFilterFunction. Now if I were to use this WebClient to make a call, the http headers carry the appropriate trace headers. The issue however is that if subsequently, the response is chained with a map or flatMap then there is no root trace available there.

Here is a test case:

        Span rootSpan = Tracer.getInstance().startRequestWithRootSpan("root");
        var webClient = WebClient.builder()
                .baseUrl("http://localhost:" + localPort)
                .filter(new WingtipsSpringWebfluxExchangeFilterFunction())
                .build();


        final Mono<Map<String, String>> mapMono = webClient.get()
                .uri("/headers")
                .accept(MediaType.APPLICATION_JSON, MediaType.APPLICATION_JSON)
                .exchange()
                .flatMap(clientResponse ->
                        clientResponse.bodyToMono(new ParameterizedTypeReference<Map<String, String>>() {
                        }))
                .map(response -> {
                    LOGGER.info("In map operation");
                    Span currentSpan = Tracer.getInstance().getCurrentSpan();
                    assertThat(currentSpan).isNotNull();
                    assertThat(currentSpan.getTraceId()).isEqualTo(rootSpan.getTraceId());
                    return response;
                });

This test would fail. The underlying reason appears to be that the thread in the map operation, is the thread used by netty under the covers which has not been instrumented with any of the tracing information.

We currently have servlet filter support split out into one for Servlet 2.x (RequestTracingFilterOldServlet from wingtips-old-servlet-api) and one for Servlet 3.x (RequestTracingFilter from wingtips-servlet-api). @adriancole suggested a solution in PR #49 on how to properly support both Servlet API environments in a single filter:

If you want to give a single filter another stab, you can borrow this approach: https://github.com/openzipkin/brave/blob/master/instrumentation/servlet/src/main/java/brave/servlet/ServletRuntime.java Basically put servlet 3 as a "provided" dep, or otherwise such that it doesn't mess up the dep graph of the caller. The detection in here will only reference the listener class if servlet 3+ is in the runtime path, otherwise it is ignored and won't interfere with 2.x (requires an integration test that removes 3.x dep to make sure, but that passes)

SpanParser is sometimes throwing an ConcurrentModificationException when too many timestamped annotations are added to the Span. Will provide a merge request using a SynchronizedList for timestampedAnnotations shortly.

Wingtips currently uses decimal number encoding when generating its 64 bit random trace and span IDs. Zipkin uses hex encoding when propagating IDs between systems. Although Wingtips can currently act as an intermediary between zipkin tracing systems and everything will work fine, any IDs generated by wingtips will fail to propagate to a zipkin system. This is a huge portion of the tracing ecosystem that wingtips would be unnecessarily locked out of.

Wingtips should switch to hex encoding for its IDs for full B3 compatibility as described here: http://zipkin.io/pages/instrumenting.html. The X-B3-Sampled header should also be propagated as "1" for true, and "0" for false.

The wingtips-apache-http-client library contains an interceptors for the HttpClient. While the interceptors implement the interfaces necessary for Apache's HttpAsyncClient, they work properly in the async client.

The existing WingtipsApacheHttpClientInterceptor starts new subspans via the request interceptor on the current thread (from which the request is executed) but when the response is handled, the async client executes the response interceptor on a different thread. This causes the subspan to never be closed and produces errors when the parent span is finalized.

I have been trying out Wingtips with Zipkin today. I love the docs and overall simplicity of use. I created simple client-server app. Client is written using Cujojs-rest Zipkin instrumentation and Server is created in Spark web framework. When I make request from client to server, and Wingtips tries to convert strings extracted from headers to long it always ends in NumberFormatException in WingtipsToZipkinSpanConverterDefaultImpl in nullSafeLong() method. The solution I found working was to add this:

protected Long nullSafeLong(String str) {
        if (str == null)
            return null;

        long longString = new BigInteger((str), 16).longValue();
        return longString;
    }