Not all client to client relations taken into account in 'Dependencies' tree

[Tomasz Michnik]

Hi,

I am writing on the forum as github/issues is not the best place for discussion. 

We have a question regarding https://github.com/openzipkin/zipkin/issues/2616.

It was said that removing a condition from line 70 (zipkin2.internal.DependencyLinker) is too dangerous and might existing application to blow up,

but maybe it is possible to make it configurable?

I mean if someone wants to have all dependencies visible including internal CLIENT -> CLIENT -> CLIENT calls,

he might have a chance to switch it on.

//line 70 zipkin2.internal.DependencyLinker

if (Kind.CLIENT.equals(kind) && !current.children().isEmpty()) {

   continue;

}

What do you think about that?

We can contribute and make this change if you believe it makes sense. 

Kind Regards

[Adrian Cole]

Thanks for bringing the conversation here.

It is best to start with explaining why your code is creating client
-> client -> client links as it looks like a bug. For example, why are
your apps creating this type of data and did you ask the owner of
whatever is creating this data to correct it? It is possible there is
an alternative besides changing the library used by everyone, possibly
there is a much better option. Remember you are asking for us to
change the code used by everyone: this is the last choice not the
first choice.

To continue this topic, please provide a minimal trace that captures a
trace like this, and mention which library is creating that. For
example, you could paste a formatted json trace minus any secrets to
https://gist.github.com or another public paste bin.

cheers,
-Adrian

> --
>
> ---
> 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/fe33bb7e-43df-45f7-8820-186d0d6f0cac%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Tomasz Michnik]

Hi,

You are absolutely  right! I should have started with explanations where those client->client spans came from.

We are working on a set of tools/libraries which is used by other developers within our company (huge bank).

 

Those client spans are generated on purpose as we want to monitor different layers of our applications. It gives us the whole view what a particular call/process looks like.  

We use twitter's finagle libs for http requests. Moreover, we created some custom filters extending com.twitter.finagle.Filter.

One of those custom filters, generates  the additional spans.  

On a attached picture (spans-1.png) you will find a tree of spans with my captions. 

In a nutshell, 

"demo" - custom software client - generates span with a filter

"auth" - finagle client calling the authorization service

"service1" -  finagle client calling our demo server app

"okapidemo" - calls from "service1" goes through TracingFilter on a server side

I do not have any more realistic samples as we are on our way to introduce Zipkin to our organization.  

Tom

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

[Brian Devins-Suresh]

Ok, so I think this is a modeling problem then.

The model of zipkin would say that instead of `localEndpoint` getting changed for each thing you call, you would set `remoteEndpoint`. Local endpoint should always be the application (process) that the span is created in.

Does that make sense?

- Brian

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/5cb0226d-7202-4acd-bd3a-50f1874f17e7%40googlegroups.com.

[Adrian Cole]

Agree with brian.

Also, if it is finagle code that isn't setting the endpoints
correctly, it could be possible to fix it locally with zipkin-finagle.
Can you verify that the apps sending data to zipkin with finagle are
using zipkin-finagle to do so?

https://github.com/openzipkin/zipkin-finagle-example

> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/CAO3-Ny6ekfiYxL67c%2BNyzSbswD2sZK5qEXgzgZcYb4G63Pr5_g%40mail.gmail.com.

[Adrian Cole]

Luckily fixing labels in finagle isn't that bad especially if you keep
your client configuration tidy.

// Ex. most commonly people set the label of the client to the
downstream, which finagle's tracer uses as the local service name
// Set "label" to the same as everything else in the process. Ex if
the process is named "frontend", do this.
Service<Request, Response> backendClient = Http.client()
.withTracer(tracer)
.withLabel("frontend") // this assigns the local service name
.newService("localhost:9000");


Meanwhile, upvote the following issue which we can use to fix bad data
before it gets to zipkin. Note: you would need to use recent finagle
if we implemented this:
https://github.com/openzipkin/zipkin-finagle/issues/67

[Tomasz Michnik]

Adrian, just to confirm, we do use zipkin-finagle on a client side. 

Basically, what you've guys said about a model explains a lot.

Unfortunately, our case is slightly more complicated and we cannot tackle it just by simply setting a label.

Let us assume, we have an application X which calls 3 other services: A, B, C and for that purpose creates 3 separate instances of  com.twitter.finagle.Service.

Those finagle service instances have labels' values set to the target services' names accordingly. 

As far as I understand, you suggested to set label X on those 3 client service instances.     

I do not think (at least in our case) it is a right approach as according to docs:

 

// com.twitter.finagle.Client

 /** 

 *

 * @define label

 *

 * Argument `label` is used to assign a label to this client.

 * The label is used to display stats, etc.

 */

trait Client[Req, Rep] {

  /** $newService $label */

  def newService(dest: Name, label: String): Service[Req, Rep]

  /** $newService */

  final def newService(dest: String): Service[Req, Rep] = {

    val (n, l) = Resolver.evalLabeled(dest)

    newService(n, l)

  }

  /** $newService */

  final def newService(dest: String, label: String): Service[Req, Rep] =

    newService(Resolver.eval(dest), label)

  /* ......... */

}

finagle uses that label for stacs. 

We use those statistics and already have working monitoring which gives us information about target service causing problems.

We have made test, and it turned out that  setting  label  fixed local serve name but on the other hand  messed up our statistics. 

However, if we had a possibility to set local service name directly on  zipkin-finagle Tracers that would suit our needs. 

I guess, this is what you want to achieve in https://github.com/openzipkin/zipkin-finagle/issues/67 ?  

If so, we will have to wait until it is implemented? Please, confirm. 

BTW with  brave.Tracing it is possible to that by simply passing  local service name the builder.

Tracing.newBuilder().supportsJoin(false)

                .localServiceName("your-local-service-name")

                .spanReporter(reporter).build();

W dniu środa, 3 lipca 2019 02:23:46 UTC+2 użytkownik Adrian Cole napisał:

Luckily fixing labels in finagle isn't that bad especially if you keep
your client configuration tidy.

// Ex. most commonly people set the label of the client to the
downstream, which finagle's tracer uses as the local service name
// Set "label" to the same as everything else in the process. Ex if
the process is named "frontend", do this.
Service<Request, Response> backendClient = Http.client()
    .withTracer(tracer)
    .withLabel("frontend") // this assigns the local service name
    .newService("localhost:9000");


Meanwhile, upvote the following issue which we can use to fix bad data
before it gets to zipkin. Note: you would need to use recent finagle
if we implemented this:
https://github.com/openzipkin/zipkin-finagle/issues/67

> >> To view this discussion on the web visit https://groups.google.com/d/msgid/zipkin-user/5cb0226d-7202-4acd-bd3a-50f1874f17e7%40googlegroups.com.
> >> For more options, visit https://groups.google.com/d/optout.
> >
> > --
> >
> > ---
> > 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 zipki...@googlegroups.com.

[Adrian Cole]

Thanks for the explanation. I don't think waiting for the code change
will be a problem https://github.com/openzipkin/zipkin-finagle/pull/68

> 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/a780eeb7-1ee6-4543-b176-b9c043793280%40googlegroups.com.

[Tomasz Michnik]

Hi,

Regarding https://github.com/openzipkin/zipkin-finagle/pull/68, when are you going to release a new version of zipkin-finagle?

Tom

[Adrian Cole]

the change you commented on is all good, but I would like chris'
opinion as he is most often releasing this
https://github.com/openzipkin/zipkin-finagle/pull/69 needs some unit
tests then that can merge.

considering I'm personally on some international travel right now "by
saturday" one way or another should be a fair estimate.

> 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/80b96ad1-13cb-4bb3-ba0d-74e63b76128b%40googlegroups.com.