Makes Lens more tolerant of missing localEndpoint.serviceName or .name #2834

codefromthecrypt · 2019-10-07T15:07:46Z

See #2829

devinsba · 2019-10-07T15:29:23Z

Should we add the offending trace to the folder of sample data?

codefromthecrypt · 2019-10-07T15:54:15Z

horrible-trace.json? :D

codefromthecrypt · 2019-10-07T16:36:08Z

Now unknown instead of undefined in trace list

Now, no more white screen rendering this really bad trace:

codefromthecrypt · 2019-10-07T16:38:52Z

PS it is tempting to make the span-cleaner fill empty names and such, but it actually would spread debt into other things like clock skew and dependency linker which looks for presence of service names. Instead, I made it a goal to make the white screen go away.

codefromthecrypt · 2019-10-07T16:40:42Z

PPS here, Kong should have used an annotation, not an empty span except for timestamp and name. While definitely bad data, I wasn't able to figure out how to get the warning about "no rows found" to go away.

codefromthecrypt

@bungle @jeremyjpj0916 the comments on this part of the review are some hints about data problems for Kong/kong-plugin-zipkin#49 Hope they help.

codefromthecrypt · 2019-10-07T16:45:27Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "d65495313f944801",
+    "id": "d5cb70daead478dc",
+    "name": "kong.header_filter",
+    "timestamp": 1570005203864000


this should have been an annotation

Could you expand on this a little bit? The Zipkin docs make it very clear that the timestamp field exists, and also say:

The bottom-line is that choosing not to record Span.timestamp and duration will result in less accurate data and less functionality. Since it is very easy to record these authoritatively before reporting, all Zipkin instrumentation should do it or ask someone to help them do it.

Wouldn't removing the timestamp field making Zipkin less accurate?

Also: can I name an annotation timestamp?

I think what's missing here is that a span can include annotations.. annotations have a required timestamp. So instead of making a sub-span that only includes a name and a timestamp, make an annotation with that name and timestamp.

ex

{ "traceId": "6cf5624e3452d1bc202a57dad35921c0", "parentId": "ed395cb866d02665", "id": "d65495313f944801", "kind": "CLIENT", "name": "get /users/{userId}", "timestamp": 1570005203829000, "duration": 35000, --snip-- "annotations": [ {"name": "kong.header_filter", "timestamp": 1570005203864000} ]

codefromthecrypt · 2019-10-07T16:45:32Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "d65495313f944801",
+    "id": "80b8efd52d66af27",
+    "name": "kong.body_filter",
+    "timestamp": 1570005203864000


this should have been an annotation

These are spans in Kong; I recall that when they execute in under a millisecond then they appear as a 0-length span in zipkin.

minimum duration is 1us, so prefer to use that vs rounding down to zero, especially if precision is millis (1000us) on the kong side. Zero implies incomplete.

The main thing is to differentiate annotations (timestamped events on a timeline) from spans (some wrapped thing with a duration).

codefromthecrypt · 2019-10-07T16:45:35Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "ed395cb866d02665",
+    "id": "2a49858b1f1d506b",
+    "name": "kong.rewrite",
+    "timestamp": 1570005203829000


this should have been an annotation

codefromthecrypt · 2019-10-07T16:45:54Z

zipkin-lens/src/test/data/malformed.json

+    "traceId": "6cf5624e3452d1bc202a57dad35921c0",
+    "parentId": "d65495313f944801",
+    "id": "3e53779bdde7c111",
+    "name": "kong.access",


this is missing localEndpoint.serviceName or this should be an annotation, and the duration moved to where it is missing..

By looking at the examples in Zipkin's source code, I think this means that the span should look like so:

{ "traceId": "6cf5624e3452d1bc202a57dad35921c0", "parentId": "d65495313f944801", "id": "3e53779bdde7c111", "name": "kong.access", "localEndpoint": { "serviceName": "<name of the Kong Service matched for this Request>", }, "timestamp": 1570005203829000, "duration": 16000 },

I think I can try to add that. But I have questions:

Is localEndpoint.serviceName also needed in the other spans, or just on this one?

What does "the duration moved to where it is missing" mean? That the other spans should also have a duration? That the duration field on this particular span should be put in a different place on this span (where)? Or is it both?

the client span should be complete, that's the most important one, but all spans should be complete. The subspans, some of them have duration. It was unclear to me why these are modeled as spans vs events.

all spans should have the exact same localEndpoint value if coming from the same node. Most of the time folks just take the link local IP, the advertised port and service name and treat that as a constant.

codefromthecrypt · 2019-10-07T16:46:12Z

zipkin-lens/src/test/data/malformed.json

+    "tags": {
+      "kong.route": "aaa20d62-813a-428f-aa40-447324cd30af",
+      "kong.service": "872eae11-4571-490b-86d2-59834b93f870",
+      "peer.hostname": ""


don't add empty hostname

codefromthecrypt · 2019-10-07T16:48:09Z

zipkin-lens/src/test/data/malformed.json

+    "traceId": "6cf5624e3452d1bc202a57dad35921c0",
+    "id": "ed395cb866d02665",
+    "kind": "SERVER",
+    "name": "kong.request",


bad span name. prefer route-based ex get /users/{userId} or even get as kong is already tagged

codefromthecrypt · 2019-10-07T16:48:33Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "ed395cb866d02665",
+    "id": "d65495313f944801",
+    "kind": "CLIENT",
+    "name": "kong.proxy",


bad span name. prefer route-based ex get /users/{userId} or even get as kong isn't helpful

what you are describing here, seems to be better as a tag

Does this apply to the other span names as well? (kong.access, kong.header_filter, kong.body_filter, kong.rewrite, kong.access) or is it just kong.proxy?

the client span should be semantically correct for http. It is better to have a coherent client span than any of the other spans mentioned. As you noticed, they didn't render and actually made the diagram more confusing vs if they didn't exist at all. My advice is probably use one client span and render the rest with annotations if it is commonly the case you cannot record a duration of precision less than 1ms.

codefromthecrypt · 2019-10-07T16:48:49Z

zipkin-lens/src/test/data/malformed.json

+      "component": "kong",
+      "http.method": "GET",
+      "http.status_code": "200",
+      "http.url": "",


don't add empty url

codefromthecrypt · 2019-10-07T16:49:17Z

zipkin-lens/src/test/data/malformed.json

+      "port": 3771
+    },
+    "tags": {
+      "component": "kong",


conventionally zipkin uses "lc" for "local component"

codefromthecrypt · 2019-10-07T16:50:03Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "d65495313f944801",
+    "id": "862d0f978033cc53",
+    "name": "kong.balancer",
+    "timestamp": 1570005203845000,


missing duration. probably better as an annotation named kong.balancer.try1 as that should be bounded, unless there's a chance of infinite tries. That or make this the client span if this is the actual wire request.. hard to tell what's going on (might need a wiki)

codefromthecrypt · 2019-10-08T13:06:29Z

verified one of the envoy traces renders properly eventhough it is missing the local service name:

kikito

Hi @adriancole, I am trying to fix the issues reported on the Kong plugin. Thanks for reporting the problems to us. I left some questions on this PR, do you think you could answer those?

kikito · 2019-10-10T14:56:00Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "d65495313f944801",
+    "id": "d5cb70daead478dc",
+    "name": "kong.header_filter",
+    "timestamp": 1570005203864000


Could you expand on this a little bit? The Zipkin docs make it very clear that the timestamp field exists, and also say:

The bottom-line is that choosing not to record Span.timestamp and duration will result in less accurate data and less functionality. Since it is very easy to record these authoritatively before reporting, all Zipkin instrumentation should do it or ask someone to help them do it.

Wouldn't removing the timestamp field making Zipkin less accurate?

Also: can I name an annotation timestamp?

kikito · 2019-10-10T14:58:49Z

zipkin-lens/src/test/data/malformed.json

+    "traceId": "6cf5624e3452d1bc202a57dad35921c0",
+    "parentId": "d65495313f944801",
+    "id": "3e53779bdde7c111",
+    "name": "kong.access",


By looking at the examples in Zipkin's source code, I think this means that the span should look like so:

{ "traceId": "6cf5624e3452d1bc202a57dad35921c0", "parentId": "d65495313f944801", "id": "3e53779bdde7c111", "name": "kong.access", "localEndpoint": { "serviceName": "<name of the Kong Service matched for this Request>", }, "timestamp": 1570005203829000, "duration": 16000 },

I think I can try to add that. But I have questions:

Is localEndpoint.serviceName also needed in the other spans, or just on this one?

What does "the duration moved to where it is missing" mean? That the other spans should also have a duration? That the duration field on this particular span should be put in a different place on this span (where)? Or is it both?

kikito · 2019-10-10T15:12:03Z

zipkin-lens/src/test/data/malformed.json

+    "parentId": "ed395cb866d02665",
+    "id": "d65495313f944801",
+    "kind": "CLIENT",
+    "name": "kong.proxy",


Does this apply to the other span names as well? (kong.access, kong.header_filter, kong.body_filter, kong.rewrite, kong.access) or is it just kong.proxy?

Source: openzipkin/zipkin#2834 (comment)

codefromthecrypt mentioned this pull request Oct 7, 2019

Blank page when click detail trace at Lens Zipkin #2829

Closed

Coerces missing values when rendering span rows

a209cea

codefromthecrypt force-pushed the tolerate-bad-data branch from 2d46a79 to a209cea Compare October 7, 2019 16:34

codefromthecrypt marked this pull request as ready for review October 7, 2019 16:35

codefromthecrypt requested review from tacigar and anuraaga October 7, 2019 16:35

codefromthecrypt mentioned this pull request Oct 7, 2019

Numerous data problems Kong/kong-plugin-zipkin#49

Closed

codefromthecrypt commented Oct 7, 2019

View reviewed changes

anuraaga approved these changes Oct 8, 2019

View reviewed changes

adds envoy trace to test case

2a0faaf

codefromthecrypt merged commit 7838a33 into master Oct 8, 2019

codefromthecrypt deleted the tolerate-bad-data branch October 8, 2019 13:42

kikito reviewed Oct 10, 2019

View reviewed changes

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Oct 16, 2019

feat(reporter) rename component tag to lc

ad051e4

Source: openzipkin/zipkin#2834 (comment)

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Oct 25, 2019

feat(reporter) rename component tag to lc

8534cbb

Source: openzipkin/zipkin#2834 (comment)

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Oct 25, 2019

feat(reporter) rename component tag to lc

6cd8388

Source: openzipkin/zipkin#2834 (comment)

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Oct 30, 2019

feat(reporter) rename component tag to lc

a1e17d2

Source: openzipkin/zipkin#2834 (comment)

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Oct 31, 2019

feat(reporter) rename component tag to lc

18ef963

Source: openzipkin/zipkin#2834 (comment)

kikito added a commit to Kong/kong-plugin-zipkin that referenced this pull request Nov 5, 2019

feat(reporter) rename component tag to lc

c8538b3

Source: openzipkin/zipkin#2834 (comment)

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

feat(zipkin) rename component tag to lc

79d3450

Source: openzipkin/zipkin#2834 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes Lens more tolerant of missing localEndpoint.serviceName or .name #2834

Makes Lens more tolerant of missing localEndpoint.serviceName or .name #2834

codefromthecrypt commented Oct 7, 2019

devinsba commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019 •

edited

Loading

codefromthecrypt left a comment

codefromthecrypt Oct 7, 2019

kikito Oct 10, 2019

codefromthecrypt Oct 11, 2019

codefromthecrypt Oct 7, 2019

james-callahan Oct 10, 2019

codefromthecrypt Oct 11, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt Oct 7, 2019

kikito Oct 10, 2019

codefromthecrypt Oct 11, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt Oct 7, 2019

kikito Oct 10, 2019

codefromthecrypt Oct 11, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt Oct 7, 2019

codefromthecrypt commented Oct 8, 2019

kikito left a comment

kikito Oct 10, 2019

kikito Oct 10, 2019

kikito Oct 10, 2019

Makes Lens more tolerant of missing localEndpoint.serviceName or .name #2834

Makes Lens more tolerant of missing localEndpoint.serviceName or .name #2834

Conversation

codefromthecrypt commented Oct 7, 2019

devinsba commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019

codefromthecrypt commented Oct 7, 2019 • edited Loading

codefromthecrypt left a comment

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codefromthecrypt commented Oct 8, 2019

kikito left a comment

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 7, 2019 •

edited

Loading