Calls server to client not visible on 'Dependencies' tree

[Tomasz Michnik]

Hi,

Regarding a  ticket (https://github.com/openzipkin/zipkin/issues/2640), we have doubts about the generated dependencies. 

zipkin: 2.12.9

I prepared 2 short tests for zipkin2.internal.DependencyLinker for better understanding what I mean.

In both cases, links server -> client1 are missing. Is there anything wrong with my entry data or tests?

@test

public void shouldServerClientRemoteCallsWithServerDescendantBuildLinks() {

List trace = asList(

span2("a", null, "a", Kind.CLIENT, "client0", null, false),

span2("a", "a", "b", Kind.SERVER, "server", "client0", false),

span2("a", "b", "c", Kind.CLIENT, "client1", null, false),

span2("a", "c", "d", Kind.SERVER, "server1", "client1", false)

);

assertThat(new DependencyLinker().putTrace(trace).link()).contains(

  DependencyLink.newBuilder().parent("client0").child("server").callCount(1L).build(),

  DependencyLink.newBuilder().parent("server").child("client1").callCount(1L).build(),

  DependencyLink.newBuilder().parent("client1").child("server1").callCount(1L).build()

);

}

@test

public void shouldServerClientRemoteCallsWithClientDescendantBuildLinks() {

List trace = asList(

span2("a", null, "a", Kind.CLIENT, "client0", null, false),

span2("a", "a", "b", Kind.SERVER, "server", "client0", false),

span2("a", "b", "c", Kind.CLIENT, "client1", null, false),

span2("a", "c", "d", Kind.CLIENT, "client2", null, false)

);

assertThat(new DependencyLinker().putTrace(trace).link()).contains(

  DependencyLink.newBuilder().parent("client0").child("server").callCount(1L).build(),

  DependencyLink.newBuilder().parent("server").child("client1").callCount(1L).build(),

  DependencyLink.newBuilder().parent("client1").child("client2").callCount(1L).build()

);

BR,

Tomasz

[Adrian Cole]

this is not answering the question I asked. this is a test case..

what is the production data causing this data coming from? do you have
an example trace that motivated your question in the first place?

> --
>
> ---
> You received this message because you are subscribed to the Google Groups "zipkin-user" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/b62f8572-c85d-41da-95ed-39fa28e4e06c%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Adrian Cole]

ps it is hard to explain in email with unit tests because not all of the important things are in the email. for example in the tests you are showing the third last parameter is the local service name and the second to last is the remote service name 

https://github.com/openzipkin/zipkin/blob/master/zipkin/src/test/java/zipkin2/internal/DependencyLinkerTest.java#L577

the data provided is an example of misconfigured or buggy instrumentation, or manually created data that isnt mapped properly.

@test

public void shouldServerClientRemoteCallsWithServerDescendantBuildLinks() {

List trace = asList(

span2("a", null, "a", Kind.CLIENT, "client0", null, false),

span2("a", "a", "b", Kind.SERVER, "server", "client0", false),

span2("a", "b", "c", Kind.CLIENT, "client1", null, false),

span2("a", "c", "d", Kind.SERVER, "server1", "client1", false)

);

this literally says...

span a->b .. client: client0 calls server: server

this is ok

span b->c somehow the local service name changed to client1? how could that have happened..  it was named server just one span ago?

this starts what appears to be a configuration bug and it is hard to type more about it. your local service name will stay the same until you hit the next service. in your test case, it flips.

you can look at examples like sleuth-webmvc-example or zipkin-js-example.

if you are a client span, unless you are the root span, your local endpoint service name should be the same as your parent.

that is why I ask what is creating this bad data.

[Tomasz Michnik]

Hi,

You were right about the model. In fact there is an issue with local service name (https://groups.google.com/forum/embed/#!topic/zipkin-user/EgTN97wO4Sk).

However, I found another problem  which troubles me.

Providing that,  "local service name" issue is fixed we might have a situation where :

1. Service 1 -> Service 2  //OK
2. Service 1 -> Authorization service (not-instrumented with Zipkin)
3. Service 2 -> Service 3 //OK

On the client side we use zipkin-finagle to build and send spans. 

It turned out that from some reason  zipkin-finagle never generates spans with remoteEndpoint.serviceName which is necessary to build relations (dependency) with uninstrumented services.

Without remoteEndpoint.serviceName DependencyLinker does not have a clue about 3rd party service which is not instrumented and of course does not produce spans. 

[Adrian Cole]

On the remote service name, you are right that this is not going to
work well until a small change occurs in Finagle.

zipkin-js has the same api style as finagle. What we did there was add
an attribute to ServerAddr of "serviceName"

function ServerAddr({serviceName, host, port}) {

That's not currently the case in Finagle, but it would be a very small
code change so they might do it quickly.

final case class ServerAddr(ia: InetSocketAddress) extends Annotation

https://github.com/twitter/finagle/blob/develop/finagle-core/src/main/scala/com/twitter/finagle/tracing/Annotation.scala#L31

It is the case that some change in finagle takes a long time, but it
is still worth raising an issue. Can you do that? It is better when it
comes from a user

https://github.com/twitter/finagle/issues/new

> --
>
> ---
> You received this message because you are subscribed to the Google Groups "zipkin-user" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/f44124ac-0f1d-466c-81f6-d5d8eadc55a7%40googlegroups.com.

[Adrian Cole]

One option Brian mentioned on the github issue is.. what if we take
the client's serviceName value and move it to the remoteServiceName
when the zipkin.localServiceName flag is set. Worst case, we get a bad
local service name set as a remote service name. However, since this
flag is opt-in anyway, it could make sense..

https://github.com/openzipkin/zipkin-finagle/pull/68#discussion_r300389213

[Adrian Cole]

Hi, Tomasz

Let's please come to closure on this topic. Can you please try the change here?

https://github.com/openzipkin/zipkin-finagle/pull/68#issuecomment-508735756

Best,

-A

> > To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user+unsubscribe@googlegroups.com.

[Tomasz Michnik]

Hi,

Obviously, I can raise a ticket but before that I need some clarification.

 

As far as I understand you want Zipkin guys to add param serviceName  to a constructor?

 object Annotation {

final case class ServerAddr(ia: InetSocketAddress) extends Annotation
}

But how does it help? It seems to me that it is not even invoked. So I guess it is not the only change needed? 

 

I forgot to mention that I was trying to add ServerAddr manually in my integration tests however it was ignored in 

//zipkin2.v1.V1SpanConverter

  void processBinaryAnnotations(V1Span source) {

    zipkin2.Endpoint ca = null, sa = null, ma = null;

    for (int i = 0, length = source.binaryAnnotations.size(); i < length; i++) {

      V1BinaryAnnotation b = source.binaryAnnotations.get(i);

      if (b.stringValue == null) { // <=== this caused ignoring my server endpoint

        if ("ca".equals(b.key)) {

          ca = b.endpoint;

        } else if ("sa".equals(b.key)) {

          sa = b.endpoint;

        } else if ("ma".equals(b.key)) {

          ma = b.endpoint;

        }

        continue;

      }

/*..........*/
}

It looks that zipkin2.v1.V1SpanConverter.processBinaryAnnotations(V1Span source) is a part where annotations are eventually processed and remote endpoint is added to  zipkin2.Span.
 

> To unsubscribe from this group and stop receiving emails from it, send an email to zipki...@googlegroups.com.

[Adrian Cole]

The skipping of the address info was an oversight/bug, not an intent.
Incidentally I fixed that as a part of the change pending your review
https://github.com/openzipkin/zipkin-finagle/pull/68/files#diff-baf789ea4172b4880ca4dadd282afdaaR130

> To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/036e6b73-b7d0-4df7-b801-7c51f71ace3d%40googlegroups.com.

[Tomasz Michnik]

OK I will try to test it out.

W dniu piątek, 5 lipca 2019 14:01:43 UTC+2 użytkownik Adrian Cole napisał:

Hi, Tomasz

Let's please come to closure on this topic. Can you please try the change here?

https://github.com/openzipkin/zipkin-finagle/pull/68#issuecomment-508735756

Best,

-A

On Thursday, July 4, 2019 at 9:08:52 PM UTC+8, Adrian Cole wrote:

One option Brian mentioned on the github issue is.. what if we take
the client's serviceName value and move it to the remoteServiceName
when the zipkin.localServiceName flag is set. Worst case, we get a bad
local service name set as a remote service name. However, since this
flag is opt-in anyway, it could make sense..

https://github.com/openzipkin/zipkin-finagle/pull/68#discussion_r300389213

> > To unsubscribe from this group and stop receiving emails from it, send an email to zipki...@googlegroups.com.

[Tomasz Michnik]

Hi Adrian,

I forgot to mention about 

// zipkin2.finagle.MutableSpan 

synchronized V1Span toSpan() {

    // fill in the host/service data for all the annotations

    for (V1Annotation a : annotations) {

      Endpoint ep = Endpoints.boundEndpoint(a.endpoint());

      span.addAnnotation(a.timestamp(), a.value(), ep.toBuilder().serviceName(service).build());

    }

    for (V1BinaryAnnotation ann : binaryAnnotations) {

      if (ann.stringValue() == null) continue;

      Endpoint ep = Endpoints.boundEndpoint(ann.endpoint());

      span.addBinaryAnnotation(

          ann.key(), ann.stringValue(), ep.toBuilder().serviceName(service).build());

    }

    return span.build();

  }

but I see you found it by yourself.

Am I still supposed to ask Zipkin to  add another constructor (modify existing) with serviceName param, as I can see you implemented  moveServiceNameToRemoteServiceName(v2span, oldService) ?

// com.twitter.finagle.tracing.Annotation

object Annotation {

   final case class ServerAddr(ia: InetSocketAddress) extends Annotation 
}

Kind Regards,

Tom

[Adrian Cole]

> but I see you found it by yourself.

yep that bug we discussed, with or without this change we can get the
lack or remote address sorted.

> Am I still supposed to ask Zipkin to add another constructor (modify existing) with serviceName param, as I can see you implemented moveServiceNameToRemoteServiceName(v2span, oldService) ?
>
> // com.twitter.finagle.tracing.Annotation
> object Annotation {
> final case class ServerAddr(ia: InetSocketAddress) extends Annotation
> }

It is usually best something explicit vs rely on heuristics, so yes, I
would still ask for the ability to add a service qualifier to the
remote address. This may not land for several months or at all, but it
is good to raise the issue.

The approach in the pull request can co-exist with a longer term
solution whenever that lands.

Thanks,
-A

[Tomasz Michnik]

Ok,

Issue created https://github.com/twitter/finagle/issues/778.

Assuming they will do a requested change, there might be another problem.

Condition: if (b.stringValue == null) wont be met  and our server address will be skipped.

//zipkin2.v1.V1SpanConverter

  void processBinaryAnnotations(V1Span source) {

   zipkin2.Endpoint ca = null, sa = null, ma = null;

   for (int i = 0, length = source.binaryAnnotations.size(); i < length; i++) {

     V1BinaryAnnotation b = source.binaryAnnotations.get(i);

     if (b.stringValue == null) { // <=== this caused ignoring my server endpoint

       if ("ca".equals(b.key)) {

         ca = b.endpoint;

       } else if ("sa".equals(b.key)) {

         sa = b.endpoint;

       } else if ("ma".equals(b.key)) {

         ma = b.endpoint;

       }

       continue;

     }

/*..........*/
}

At the end of processBinaryAnnotations() there is 

 if (sa != null) { // client span

      if (cs != null) {

        forEndpoint(source, cs.endpoint).remoteEndpoint(sa);

      } else if (cr != null) {

        forEndpoint(source, cr.endpoint).remoteEndpoint(sa);

      }

    }

but as our sa (server address) has been skipped, sa is null at that point.

Am I right?

[Adrian Cole]

thanks for the issue

about potential bugs, please use github comments. we have some unit tests now covering the area. it is a bit laborious to replicate code comments here and also in the pull request.

 https://github.com/openzipkin/zipkin-finagle/pull/68

--

---
You received this message because you are subscribed to the Google Groups "zipkin-user" group.

To unsubscribe from this group and stop receiving emails from it, send an email to zipkin-user...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/7e10310f-95f9-4e36-8e41-dbaddcc6b2bf%40googlegroups.com.