Consider using `localEndpoint` instead of `peer.*` tags

[jcchavezs]

peer.service, peer.ipv4, peer.ipv6 and peer.port are opentracing tags whereas zipkin uses the localEndpoint attribute:

This certainly is the case for #39 and #53

[kikito]

Hi, thanks for opening this issue as well. The current (after #52) implementation might already be doing some part of what you suggest. But I need your help filling up the gaps.

First, there's localEndpoint and remoteEndpoint. I am not sure about what they mean, or what they should mean in the context of Kong.

Assuming #57 is also merged, the each HTTP request proxied generates the following spans:

• A request span called GET for the whole thing.
• A proxy span called GET (proxy) for Kong's internal processing.
• Zero or more spans called GET (balancer try n) for each balancer try.

Of these, the behavior of localEndpoint is:

• null on the request span
• null on all the balancer try spans
• If a Kong service has been identified while proxying, the proxy's localEndpoint is { serviceName = "My-Service-Name" }.

For reference, here's how the remoteEndpoint field works, currently:

• { ipv4 = <remote address of the client making the request>, port = <client port> } on the request span
• { ipv4 = <balancer try ip>, port = <balancer try port> } on each balancer port.
• { ipv4 = <balancer success ip>, port = <balancer success port> } on proxy span.

Comments / questions:

• I read that the serviceName must be lowercase. That restriction does not apply to server names. Shall I lowercase them before sending them to Zipkin? Are there other restrictions (i.e. no spaces)?
• Should I set the localEndpoint for the request span and the balancer try spans, in addition to the proxy span?
• I take I should add ipv4/port to localEndpoint. But which ips/should should I use there? Perhaps the <remote address of the client making the request> (currently being used on the proxy span's remoteEndpoint)? That way the remoteEndpoint would point "upstream", to the services, while the localEndpoint would point "downstream", to the "client" making the request.
• It that's the correct answer, then we should move the serviceName field to the remoteEndpoint.

But perhaps I am not understanding the roles of localEndpoint and remoteEndpoint correctly.

[jcchavezs]

The local localEndpoint is basically the service itself, in this case it will be kong service. The remoteEndpoint is the endpoint to be called i.e. it involves a remote operation. If you have that information then it will be good to include, otherwise it is not mandatory to do it.

Of these, the behavior of localEndpoint is:

• null on the request span
• null on all the balancer try spans
• If a Kong service has been identified while proxying, the proxy's localEndpoint is { serviceName = "My-Service-Name" }.

localEndpoint in all these cases should be kong (with its name, IP and port) I believe.

For reference, here's how the remoteEndpoint field works, currently:
{ ipv4 = , port = } on the request span

This is correct

{ ipv4 = , port = } on each balancer port.
{ ipv4 = , port = } on proxy span.

Are these operations that involve a remote node? I guess yes, hence it is correct.

I read that the serviceName must be lowercase. That restriction does not apply to server names. Shall I lowercase them before sending them to Zipkin? Are there other restrictions (i.e. no spaces)?

I am not entirely sure about this (where did you read it?). what is true is that zipkin displays the service names in uppercase now:

Should I set the localEndpoint for the request span and the balancer try spans, in addition to the proxy span?

Yes please. That should be part of all spans.

I take I should add ipv4/port to localEndpoint. But which ips/should should I use there? Perhaps the (currently being used on the proxy span's remoteEndpoint)? That way the remoteEndpoint would point "upstream", to the services, while the localEndpoint would point "downstream", to the "client" making the request.

localEndpoint should include the service's IP. In this case, kong's IP.

It that's the correct answer, then we should move the serviceName field to the remoteEndpoint.

remoteEndpoint can have a serviceName but that should be name of the downstream service which usually we don't know (maybe in kong case is different).

In sum, if kong calls service s1 then localEndpoint.serviceName = "kong" and localEndpoint.ip4 = $(kongs' ip) and remoteEndpoint.serviceName = s1 (only if I have access to that information) and remoteEndpoint.ip4 = $(s1 IP).

[kikito]

I read that the serviceName must be lowercase [...]

I am not entirely sure about this (where did you read it?)

In https://zipkin.io/zipkin-api/, models -> Endpoint -> serviceName. It currently says (emphasis mine):

serviceName: string. Lower-case label of this node in the service graph, such as "favstar". Leave absent if unknown. This is a primary label for trace lookup and aggregation, so it should be intuitive and consistent. Many use a name from service discovery.

[kikito]

Hi, I have given this a try in #63.

Here's the "before and after" traces:

https://gist.github.com/kikito/4cca7fb37498fb65830a4f1b8ba2236c

I've got some concerns:

• For all the spans created by the plugin, the localEndpoint is now { serviceName = "kong" }, and nothing else (kong doesn't know its own IP address). It seems a bit ... unuseful?
• When a span is tagged error: false it appears red on the UI. Should I remove the error tag and only use it when there's an error?
• On the usual "API gateway" scenario, kong proxies traffic between a "downstream consumer" and an "upstream service", generating a root "request" span, with one proxy span and 1 or more balancer tries. These (now) all have the same serviceName, when Kong has identified a service to proxy to. But the ip4/port values can be very different: on the root span they point to the consumer, while on the other spans they point to the upstream service ips/ports. I think it is weird that they have the same serviceName but such different ips/ports.

[jcchavezs]

Hi @kikito, this is great stuff.

For all the spans created by the plugin, the localEndpoint is now { serviceName = "kong" }, and nothing else (kong doesn't know its own IP address). It seems a bit ... unuseful?

It is not because now you can search spans based on that serviceName.

When a span is tagged error: false it appears red on the UI. Should I remove the error tag and only use it when there's an error?

Usually we don't pass zero values, error:false is a zero value because it means that there is not error. It would be great if you can remove it.

On the usual "API gateway" scenario, kong proxies traffic between a "downstream consumer" and an "upstream service", generating a root "request" span, with one proxy span and 1 or more balancer tries. These (now) all have the same serviceName, when Kong has identified a service to proxy to. But the ip4/port values can be very different: on the root span they point to the consumer, while on the other spans they point to the upstream service ips/ports. I think it is weird that they have the same serviceName but such different ips/ports.

If kong is calling service A on behalf of an upstream service then that span should keep the localEndpoint.serviceName=kong and the IP could be kong's IP whereas the IP of service A should be the IP in the remoteEndpoint of the span (see https://zipkin.io/zipkin-api/#/default/post_spans).

Does that make sense?

[kikito]

Usually we don't pass zero values, error:false is a zero value because it means that there is not error. It would be great if you can remove it.

Done!

If kong is calling service A on behalf of an upstream service then that span should keep the localEndpoint.serviceName=kong and the IP could be kong's IP whereas the IP of service A should be the IP in the remoteEndpoint of the span (see zipkin.io/zipkin-api/#/default/post_spans).

Understood. I have moved the consumer ip/port to the tags section (updaded description on the PR). All the remoteEndpoints now point to the Kong Services now. Thanks for your feedback!