feat(zipkin) local & remote Endpoint improvements

Fixes #55

In all the Spans generated by kong:

* The `localEndpoint` is `{ serviceName = "kong" }` in all spans.

The `remoteEndpoint` has changed as follows:

* On the request (root) span, it remains as before:
  * `remoteEndpoint.ipv4/v6` and `remoteEndpoint.port` point to
    the consumer's forwarded ip and port

* On each balancer attempt span:
  * If a Kong Service was found when proxying, and that Service has a
    `name`, then `remoteEndpoint.serviceName` will be the Service Name.
  * `remoteEndpoint.ipv4/v6` and `remoteEndpoint.port` will point to
    the ip+port combination used on the span

* On the proxy span:
  * If a Kong Service was found when proxying, and that Service has a
    `name`, then `remoteEndpoint.serviceName` will be the Service Name.
  * `remoteEndpoint.ipv4/v6` and `remoteEndpoint.port` will point to
    the first successful combination of balancer attempts.

kong/plugins/zipkin/opentracing.lua

@@ -19,6 +19,20 @@ local OpenTracingHandler = {
 
 local tracer_cache = setmetatable({}, {__mode = "k"})
 
+local function tag_with_service_and_route(span)
+  local service = kong.router.get_service()
+  if service and service.id then
+    span:set_tag("kong.service", service.id)
+    local route = kong.router.get_route()
+    if route and route.id then
+      span:set_tag("kong.route", route.id)
+    end
+    if type(service.name) == "string" then
+      span:set_tag("peer.service", service.name)
+    end
+  end
+end
+
 
 function OpenTracingHandler:get_tracer(conf)
   local tracer = tracer_cache[conf]
@@ -240,6 +254,9 @@ function OpenTracingHandler:log(conf)
         span:set_tag("kong.balancer.state", try.state)
         span:set_tag("http.status_code", try.code)
       end
+
+      tag_with_service_and_route(span)
+
       span:finish((try.balancer_start + try.balancer_latency) / 1000)
     end
     proxy_span:set_tag("peer.hostname", balancer_data.hostname) -- could be nil
@@ -259,18 +276,8 @@ function OpenTracingHandler:log(conf)
     request_span:set_tag("kong.credential", ctx.authenticated_credential.id)
   end
   request_span:set_tag("kong.node.id", kong.node.get_id())
-  -- TODO: should we add these tags to the request span and/or the balancer spans?
-  if ctx.service and ctx.service.id then
-    proxy_span:set_tag("kong.service", ctx.service.id)
-    if ctx.route and ctx.route.id then
-      proxy_span:set_tag("kong.route", ctx.route.id)
-    end
-    if type(ctx.service.name) == "string" then
-      proxy_span:set_tag("peer.service", ctx.service.name)
-    end
-  elseif ctx.api and ctx.api.id then
-    proxy_span:set_tag("kong.api", ctx.api.id)
-  end
+
+  tag_with_service_and_route(proxy_span)
 
   proxy_span:finish(proxy_finish)
   request_span:finish(now)

kong/plugins/zipkin/reporter.lua

@@ -57,36 +57,24 @@ function zipkin_reporter_methods:report(span)
   zipkin_tags["component"] = nil
   zipkin_tags["lc"] = component
 
-  local localEndpoint do
-    local serviceName = zipkin_tags["peer.service"]
-    if serviceName then
-      zipkin_tags["peer.service"] = nil
-      localEndpoint = {
-        serviceName = serviceName,
-        -- TODO: ip/port from ngx.var.server_name/ngx.var.server_port?
-      }
-    else
-      -- configurable override of the unknown-service-name spans
-      if self.default_service_name then
-        localEndpoint = {
-          serviceName = self.default_service_name,
-        }
-        -- needs to be null, not the empty object
-      else
-        localEndpoint = cjson.null
-      end
-    end
-  end
+  local localEndpoint = {
+    serviceName = "kong"
+  }
 
   local remoteEndpoint do
+    local serviceName = zipkin_tags["peer.service"] or
+                        self.default_service_name -- can be nil
+
     local peer_port = span:get_tag "peer.port" -- get as number
-    if peer_port then
-      zipkin_tags["peer.port"] = nil
+    if peer_port or serviceName then
       remoteEndpoint = {
+        serviceName = serviceName,
         ipv4 = zipkin_tags["peer.ipv4"],
         ipv6 = zipkin_tags["peer.ipv6"],
-        port = peer_port, -- port is *not* optional
+        port = peer_port,
       }
+      zipkin_tags["peer.service"] = nil
+      zipkin_tags["peer.port"] = nil
       zipkin_tags["peer.ipv4"] = nil
       zipkin_tags["peer.ipv6"] = nil
     else

spec/zipkin_spec.lua

@@ -59,7 +59,7 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
     assert_is_integer(rann["krf"])
     assert.truthy(rann["krs"] <= rann["krf"])
 
-    assert.is_nil(request_span.localEndpoint)
+    assert.same({ serviceName = "kong" }, request_span.localEndpoint)
 
     -- proxy_span
     assert.same("table", type(proxy_span))
@@ -107,8 +107,13 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
       }
     })
 
+    local service = bp.services:insert {
+      name = "mock-http-service",
+    }
+
     -- kong (http) mock upstream
     route = bp.routes:insert({
+      service = service,
       hosts = { "mock-http-route" },
       preserve_host = true,
     })
@@ -174,15 +179,22 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
       ["http.status_code"] = "200", -- found (matches server status)
       lc = "kong"
     }, request_tags)
-    local peer_port = request_span.remoteEndpoint.port
-    assert.equals("number", type(peer_port))
-    assert.same({ ipv4 = "127.0.0.1", port = peer_port }, request_span.remoteEndpoint)
+    local consumer_port = request_span.remoteEndpoint.port
+    assert_is_integer(consumer_port)
+    assert.same({
+      ipv4 = "127.0.0.1",
+      port = consumer_port,
+    }, request_span.remoteEndpoint)
 
     -- specific assertions for proxy_span
     assert.same(proxy_span.tags["kong.route"], route.id)
     assert.same(proxy_span.tags["peer.hostname"], "127.0.0.1")
 
-    assert.same({ ipv4 = helpers.mock_upstream_host, port = helpers.mock_upstream_port },
+    assert.same({
+      ipv4 = helpers.mock_upstream_host,
+      port = helpers.mock_upstream_port,
+      serviceName = "mock-http-service",
+    },
     proxy_span.remoteEndpoint)
 
     -- specific assertions for balancer_span
@@ -194,11 +206,17 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
       assert.equals("number", type(balancer_span.duration))
     end
 
-    assert.same({ ipv4 = helpers.mock_upstream_host, port = helpers.mock_upstream_port },
+    assert.same({
+      ipv4 = helpers.mock_upstream_host,
+      port = helpers.mock_upstream_port,
+      serviceName = "mock-http-service",
+    },
     balancer_span.remoteEndpoint)
-    assert.is_nil(balancer_span.localEndpoint)
+    assert.same({ serviceName = "kong" }, balancer_span.localEndpoint)
     assert.same({
       ["kong.balancer.try"] = "1",
+      ["kong.route"] = route.id,
+      ["kong.service"] = route.service.id,
     }, balancer_span.tags)
   end)
 
@@ -233,21 +251,29 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
     local request_tags = request_span.tags
     assert.truthy(request_tags["kong.node.id"]:match("^[%x-]+$"))
     request_tags["kong.node.id"] = nil
+
     assert.same({
       ["http.method"] = "POST",
       ["http.path"] = "/hello.HelloService/SayHello",
       ["http.status_code"] = "200", -- found (matches server status)
       lc = "kong"
     }, request_tags)
-    local peer_port = request_span.remoteEndpoint.port
-    assert_is_integer(peer_port)
-    assert.same({ ipv4 = "127.0.0.1", port = peer_port }, request_span.remoteEndpoint)
+    local consumer_port = request_span.remoteEndpoint.port
+    assert_is_integer(consumer_port)
+    assert.same({
+      ipv4 = "127.0.0.1",
+      port = consumer_port,
+    }, request_span.remoteEndpoint)
 
     -- specific assertions for proxy_span
     assert.same(proxy_span.tags["kong.route"], grpc_route.id)
     assert.same(proxy_span.tags["peer.hostname"], "localhost")
 
-    assert.same({ ipv4 = "127.0.0.1", port = 15002 },
+    assert.same({
+      ipv4 = "127.0.0.1",
+      port = 15002,
+      serviceName = "grpc-service",
+    },
     proxy_span.remoteEndpoint)
 
     -- specific assertions for balancer_span
@@ -259,11 +285,17 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
       assert_is_integer(balancer_span.duration)
     end
 
-    assert.same({ ipv4 = "127.0.0.1", port = 15002 },
+    assert.same({
+      ipv4 = "127.0.0.1",
+      port = 15002,
+      serviceName = "grpc-service",
+    },
     balancer_span.remoteEndpoint)
-    assert.is_nil(balancer_span.localEndpoint)
+    assert.same({ serviceName = "kong" }, balancer_span.localEndpoint)
     assert.same({
       ["kong.balancer.try"] = "1",
+      ["kong.service"] = grpc_route.service.id,
+      ["kong.route"] = grpc_route.id,
     }, balancer_span.tags)
   end)
 
@@ -302,14 +334,14 @@ describe("integration tests with zipkin server [#" .. strategy .. "]", function(
       ["http.status_code"] = "404", -- note that this was "not found"
       lc = "kong"
     }, request_tags)
-    local peer_port = request_span.remoteEndpoint.port
-    assert_is_integer(peer_port)
-    assert.same({ ipv4 = "127.0.0.1", port = peer_port }, request_span.remoteEndpoint)
+    local consumer_port = request_span.remoteEndpoint.port
+    assert_is_integer(consumer_port)
+    assert.same({ ipv4 = "127.0.0.1", port = consumer_port }, request_span.remoteEndpoint)
 
     -- specific assertions for proxy_span
     assert.is_nil(proxy_span.tags)
     assert.is_nil(proxy_span.remoteEndpoint)
-    assert.is_nil(proxy_span.localEndpoint)
+    assert.same({ serviceName = "kong" }, proxy_span.localEndpoint)
   end)
 
   it("propagates b3 headers for non-matched requests", function()