Skip to content
This repository has been archived by the owner on Oct 29, 2019. It is now read-only.

Optimization: Local span layer issue #223

Merged
merged 3 commits into from
Jan 24, 2019

Conversation

zhaoyuguang
Copy link
Member

@zhaoyuguang zhaoyuguang commented Jan 24, 2019

Issue Url: apache/skywalking#2200
Local span layer and layer text is Unknown => 'Local'
Snapshot like this:
image

@zhaoyuguang zhaoyuguang changed the title Local span layer Unknown => Local Optimization: Local span layer issue Jan 24, 2019
wu-sheng
wu-sheng previously approved these changes Jan 24, 2019
Copy link
Member

@wu-sheng wu-sheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@wu-sheng wu-sheng added this to the 6.1.0 milestone Jan 24, 2019
Copy link
Member

@TinyAllen TinyAllen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to hide the span type "Local"? It is not useless.

@@ -222,7 +222,12 @@ export default class TraceMap {
.attr('text-anchor', 'start')
.attr('fill', d => this.type[d.data.layer])
.attr('stroke', d => this.type[d.data.layer])
.text(d => d.data.layer);
.text(d => {
if(d.data.type == 'Local' && d.data.layer == 'Unknown'){
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't just check unknown, because some RPC plugin may forget to set layer.

@wu-sheng
Copy link
Member

I am merging this. Please submit UI sync pr.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants