fix(*) zipkin span fields #52

kikito · 2019-10-25T10:49:40Z

Fixes #49

It is highly recommended to review this PR commit-by-commit, but notice that github's UI does not list them in the appropriate order.

The initial commits (on git log, not on github's UI) deal with the fact that the current specs were not included on the CI environment (only the linter was called) and they were run in a non-standard way (i.e. Kong was run in production mode, not in test mode).

The feat commits detail the new changes:

The Opentracing component tag is renamed to lc for zipkin
Empty tags are not sent to zipkin
The spans have also been restructured:
- Request span is the parent, proxy span is for kong internal processing, balancer span(s) for balancer tries.
- The balancer spans are now children of the request span instead of the proxy span, and they always contain the error tag, set to true or false.
- The Kong phases were previously encoded as individual spans. Now they have been converted into Opentracing logs / Zipkin annotations inside the request span and proxy span.
The spans for http traffic have been renamed:
- GET http://host:port for the request span
- GET http://host:port (proxy) for the proxy span
- GET http://host:port (balancer try n) for each balancer try

All these changes are tested on the last commit.

codefromthecrypt · 2019-10-28T02:17:02Z

README.md

+  - `kong.preread`, `start` / `finish`, `<timestamp>`
+  - `kong.header_filter`, `start` / `finish`, `<timestamp>`
+  - `kong.body_filter`, `start` / `finish`, `<timestamp>`
+  - `kong.try_[n]`, `start` / `finish`, `<timestamp>`


the try_n annotation is a nice thing for the top-level span, but each load balancer try should be a sub-span with a different span ID, as you don't want to repeat the same span ID in multiple places. each LB try is a different http request, in other words. Also the tag encoding will affect search in a bad way whereas kong.try_1 will not.

Thanks for the feedback, I have reverted everything involving the try_[n] tags. Each load balancer try is a new span.

codefromthecrypt · 2019-10-28T02:18:30Z

README.md

-  - `kong.balancer.try`
-  - `kong.balancer.state`: see [here](https://github.com/openresty/lua-resty-core/blob/master/lib/ngx/balancer.md#get_last_failure) for possible values
-  - `kong.balancer.code`
+  - `kong.try_[n].ipv4`


all of this should become a child span with kind=CLIENT

assuming there is always at least one http "try", each of these subspans should be a CLIENT span and the parent span should not. The encoding done here will make search unusable, but that's not the only reason subspans should be used. This style also breaks dependency graph linking

I did as requested. Notice that previously the load balancer spans were children of the proxy span, which was of kind=CLIENT. I have made them children of the request span now, which is the only span of kind "not CLIENT" (it is a SERVER span).

codefromthecrypt · 2019-10-28T02:31:06Z

I also tried to find an example of where a parent span added a "tryN" annotation.. I thought google did that in one of their cloud SDKs, but either it was deleted, or I can't find it.

Most typically, a simple parent span for logical in-process work, and a separate child span per attempt is all instrumentation do. This allows graph processing, which never looks at logs/annotations, to understand the success/error properties of in ingress->egress flow.

While up to you, I think the cleanest change would be to unfold the LB tries into sub-spans, with normal tag names, especially "error" and "http.status_code" etc. and the rest looks good.

This makes several changes in the tests in order to run them from Travis-CI as well as making them more similar to Kong tests for other plugins. * The tests for the deprecated API objects have been eliminated * The `TIMEOUT` variable was increased to avoid certain flakyness in some tests * Kong's `spec.helpers` have been used to start/stop kong * Kong's db is initialized using `helpers.get_db_utils` * The initial service, routes and plugins are created via blueprints

* Replace ; by , for delimiting table items * Add vertical spacing between functions * Fix some horizontal spacing issues due to removing tabs

Source: openzipkin/zipkin#2834 (comment)

Previously, each kong phase had a span. This changes them to logs (annotations in zipkin) with start and finish timestamps inside the proxy span and the request span. Each balancer try still is represented by a span, but these spans are now children of the request span instead of the proxy span. Their tags have been slightly modified too: `error` is now always present, and set to `true` or `false`. The span names for http traffic have been changed to: * `GET http://host:port` for the request span * `GET http://host:port (proxy)` for the proxy span * `GET http://host:port (balancer try n)` for each balancer try span

This change exercises the changes included in the previous commits: * Three types of spans: request, proxy & balancer * Kong phases are now encoded as annotations instead of sub-spans * Empty tags aren't sent * The component tag is renamed `lc` * The request name of http requests is like `GET http://host:port` Incidentally it creates an extra test for a situation which was not tested for at all in the past (error mode behavior was only tested in the context of the traceId tags).

kikito · 2019-10-31T13:41:39Z

@adriancole did as requested, and also added a more extensive description on the PR. Thanks for the review!

codefromthecrypt · 2019-10-31T14:19:38Z

I think overall this is positive. thanks!

jcchavezs · 2019-11-05T20:51:03Z

kong/plugins/zipkin/opentracing.lua

+        component = "kong",
+        ["span.kind"] = "server",
+        ["http.method"] = method,
+        ["http.url"] = url,


Sorry, I am late for this. We usually don't record http.url, instead we record http.path because url can leak some private information like tokens or api-key. I can open a PR for this.

Hi, we haven't released a new official version of the plugin yet, so there's still time for this. If you feel confident, please open a PR. If not, I will give it a try.

jcchavezs · 2019-11-05T21:13:10Z

kong/plugins/zipkin/opentracing.lua

-    local request_span = tracer:start_span("kong.request", {
-      child_of = wire_context;
+
+    local request_span = tracer:start_span(method .. " " .. url, {


Another one: calling a span name GET /something/123abc is a high cardinality source. In http servers we use GET /something/{id} but since kong is an api-gateway it is maybe easier to call it just GET.

Gotcha, will do. Thanks!

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See #52 (review)

kikito · 2019-11-06T14:31:44Z

Hi @jcchavezs both changes are done in #57

jcchavezs · 2019-11-06T14:38:56Z

Awesome, thanks!

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See #52 (review)

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See Kong/kong-plugin-zipkin#52 (review)

codefromthecrypt reviewed Oct 28, 2019

View reviewed changes

kikito force-pushed the fix/zipkin-span-fields branch 2 times, most recently from 7dfd9e4 to 7b55528 Compare October 30, 2019 16:09

kikito added 6 commits October 31, 2019 14:10

chore(travis) actually run tests in CI

9ee8960

style(*) restyle according to Kong standards

c2530c5

* Replace ; by , for delimiting table items * Add vertical spacing between functions * Fix some horizontal spacing issues due to removing tabs

feat(opentracing) replace kong.request span name with method url

a7969b2

feat(reporter) rename component tag to lc

18ef963

Source: openzipkin/zipkin#2834 (comment)

feat(reporter) don't send empty tags to zipkin

e177a33

kikito force-pushed the fix/zipkin-span-fields branch from 7b55528 to 919f773 Compare October 31, 2019 13:20

kikito added 2 commits October 31, 2019 14:37

kikito force-pushed the fix/zipkin-span-fields branch from 919f773 to 38d66e0 Compare October 31, 2019 13:37

hishamhm approved these changes Nov 5, 2019

View reviewed changes

kikito merged commit f67e495 into master Nov 5, 2019

kikito deleted the fix/zipkin-span-fields branch November 5, 2019 20:41

jcchavezs reviewed Nov 5, 2019

View reviewed changes

kikito added a commit that referenced this pull request Nov 6, 2019

fix(opentracing) name spans using just the method

15c7e32

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See #52 (review)

kikito mentioned this pull request Nov 6, 2019

fixes: http.path and span name #57

Merged

kikito mentioned this pull request Nov 6, 2019

Consider using localEndpoint instead of peer.* tags #55

Closed

kikito added a commit that referenced this pull request Nov 7, 2019

fix(opentracing) name spans using just the method

e8b8644

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See #52 (review)

kikito added a commit that referenced this pull request Nov 7, 2019

fix(opentracing) name spans using just the method

57111a0

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See #52 (review)

gszr pushed a commit to Kong/kong that referenced this pull request Oct 27, 2021

fix(zipkin) name spans using just the method

eeeb4df

We name spans for http traffic `GET` instead of `GET http://foo.bar/baz` in order to avoid high cardinality. See Kong/kong-plugin-zipkin#52 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(*) zipkin span fields #52

fix(*) zipkin span fields #52

kikito commented Oct 25, 2019 •

edited

codefromthecrypt Oct 28, 2019

kikito Oct 31, 2019

codefromthecrypt Oct 28, 2019

kikito Oct 31, 2019

codefromthecrypt commented Oct 28, 2019

kikito commented Oct 31, 2019

codefromthecrypt commented Oct 31, 2019

jcchavezs Nov 5, 2019

kikito Nov 5, 2019

jcchavezs Nov 5, 2019

kikito Nov 5, 2019

kikito commented Nov 6, 2019

jcchavezs commented Nov 6, 2019 via email •

edited

fix(*) zipkin span fields #52

fix(*) zipkin span fields #52

Conversation

kikito commented Oct 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Oct 28, 2019

kikito commented Oct 31, 2019

codefromthecrypt commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kikito commented Nov 6, 2019

jcchavezs commented Nov 6, 2019 via email • edited

kikito commented Oct 25, 2019 •

edited

jcchavezs commented Nov 6, 2019 via email •

edited