Display service name when showing addresses in the span detail element #1114
Conversation
|
@adriancole are there unit tests for what gets displayed in the UI? |
|
yeap formatEndpoint has unit tests in traceToMustache.test.js probably, just test that passing blank has the intended effect. Also I suspect the callsite needs to be updated to actually pass serviceName, right? Otherwise it is dead code. |
|
oh I remember this issue! We discussed and performed no action on this in the past. Last time we discussed this, we noted that having endpoint information for binary annotations isn't something we expect to carry forward in the model. Making a UI change to special case when multiple services report the same value formalizes a feature that will look "missing" when we finally get time to fix the model to disallow this. Next time you are raising an issue we have discussed and took no action on, please make reference to the former discussion and mention what has changed which would lead to us taking different action now. |
|
closing this as the issue itself hasn't changed #989 |
|
this PR is unrelated to that discussion. It is here to display service name normally encoded in SA/CA/LC, which is otherwise hidden (LC had a special handling, which with this PR becomes unnecessary). The service name in the SA/CA annotations is respected in other parts of the code, specifically in the logic that derives a single service name for a span: https://github.com/openzipkin/zipkin/blob/master/zipkin-ui/js/component_ui/traceSummary.js#L53 |
|
@yurishkuro you're right. re-opened and changing the title as this is related to "address" annotations, which are special-cased. |
codefromthecrypt
changed the title
|
Ps can you put in a shot of the whole span detail element? It would be
easier to tell what the result looks like (ex if we are still consistent)
|
|
ps I caused this to slow down, and am sorry about that. In repentance, I'll add the missing tests etc. |
|
I would rather not show the parentheses at all if serviceName is empty, isn't that better? |
|
@eirslett agreed |
codefromthecrypt
force-pushed
the
display-service-name-for-endpoints
branch
from
8341bfb to
c984c9e
Compare
|
Added tests and a more complete screen shot. One glitch is that this causes the Annotations pane to show redundant info, as it has service listed already...
|
|
so I don't think this is shippable without revisions because it looks really chatty now. If this change was isolated to just the bottom part, or we took out the Service column in the top part, it would be shippable imho |
|
I guess another option would be to change the code so that it didn't apply to both sections. |
|
Yet anothet option could be to just add a service column at the bottom like there is at the top |
|
Added a commit with one option, which is to make the top match the bottom. Here's a screenshot
|
|
LGTM!
No strong opinions here. Leaving it as it is (showing the zero) indicates that Zipkin is indeed able to show which port a service answered on - which can be handy for people to know, when they try to improve their instrumentation/pass in more data. (port, in this case) |
|
lgtm. Thanks for improvements & tests. I was porting this change from our pre-JS fork where it was only affecting the bottom part. |
#1114) * Displays service name when showing addresses in the span detail element * Takes out redundant service name column
Example screenshot:
Why: when a request comes from or goes to uninstrumented service, the instrumented part may log CA/SA annotations that include not only the peer IP address, but also the service name. However, the current UI, while it has a special handing for CA/SA annotations, does not display the service name.