Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort span tags in alphabetical order #489

Merged
merged 5 commits into from
Dec 10, 2019

Conversation

nabam
Copy link
Contributor

@nabam nabam commented Nov 28, 2019

Which problem is this PR solving?

Resolves #218
Expanding a span on the UI shows collapsed list of tags which can be expanded again. When there are many tags it's hard to find the one a person is looking for

Short description of the changes

  • Trace span tags are getting sorted after deduplication in reducer
  • New option topTagPrefixes?: string[] introduced in UI configuration file. Tags with keys matching this prefixes will be displayed at the top preserving order of prefixes.

Signed-off-by: Lev Popov <leo@nabam.net>
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #489 into master will increase coverage by 0.02%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
+ Coverage   92.71%   92.74%   +0.02%     
==========================================
  Files         193      193              
  Lines        4655     4672      +17     
  Branches     1121     1126       +5     
==========================================
+ Hits         4316     4333      +17     
  Misses        299      299              
  Partials       40       40
Impacted Files Coverage Δ
...kages/jaeger-ui/src/model/transform-trace-data.tsx 88.09% <94.73%> (+3.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36ab156...129d8ac. Read the comment docs.

@nabam nabam force-pushed the sort-tags branch 3 times, most recently from 098f54a to d31f2ec Compare November 28, 2019 14:29
Signed-off-by: Lev Popov <leo@nabam.net>
Signed-off-by: Lev Popov <leo@nabam.net>
@nabam
Copy link
Contributor Author

nabam commented Nov 28, 2019

I'm not very familiar with javascript, react and redux. Please let me know if I'm doing something bizarre.

@@ -66,6 +66,7 @@ export default deepFreeze(
gaID: null,
trackErrors: true,
},
topTagPrefixes: ['http.'],
Copy link
Member

Choose a reason for hiding this comment

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

the priority sorting feels contrary to the intention of this PR. Is it really necessary?

Copy link
Contributor Author

@nabam nabam Dec 2, 2019

Choose a reason for hiding this comment

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

Sorting by itself makes it much easier to find relevant information in a trace compare to experience with tags in the random order. At least the tag one is trying to follow is at the same position in every span.

I was trying to find a way to access tags that that are more relevant than others faster. Issue #218 mentions that and in my installation I have a similar problem
Screenshot 2019-12-02 at 10 01 14
Without priority sorting I would have to name tags the way they are alphabetically in order of importance to get most relevant ones in that view.

Though I bet there are more elegant solutions for that UX problem than priority sorting. I think it's quite a standard approach to have many tags some of which are used rarely for things like troubleshooting corner cases and others are crucial for most of the usage scenarios.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the priority sorting as a config option, but I wouldn't add a default value.

Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

Thanks for the diff @nabam! I left some small change requests, but otherwise it looks good to me.

packages/jaeger-ui/src/model/transform-trace-data.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/model/transform-trace-data.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/model/transform-trace-data.tsx Outdated Show resolved Hide resolved
@@ -32,6 +33,24 @@ function deduplicateTags(spanTags: Array<KeyValuePair>) {
return { tags, warnings };
}

export function orderTags(spanTags: Array<KeyValuePair>, topPrefixes: Array<string>) {
const orderedTags: Array<KeyValuePair> = [...spanTags];
orderedTags.sort((a, b) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be case insensitive, so below this I would add:

const aKey = a.key.toLowerCase();
const bKey = b.key.toLowerCase();

and then use aKey and bKey in place of a.key and b.key

and above tree.walk on line 111 I would add:

const prefixes = getConfigValue('topTagPrefixes');
const topTagPrefixes = prefixes && prefixes.map(p => p.toLowerCase())`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that toLowerCase mapping can be done inside orderTags. It's a bit of performance loss but makes code more testable.

@@ -66,6 +66,7 @@ export default deepFreeze(
gaID: null,
trackErrors: true,
},
topTagPrefixes: ['http.'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the priority sorting as a config option, but I wouldn't add a default value.

packages/jaeger-ui/src/model/transform-trace-data.tsx Outdated Show resolved Hide resolved
packages/jaeger-ui/src/model/transform-trace-data.tsx Outdated Show resolved Hide resolved
@nabam nabam force-pushed the sort-tags branch 2 times, most recently from fc4377b to a1ccb29 Compare December 5, 2019 10:01
@nabam nabam requested a review from everett980 December 5, 2019 10:02
@nabam nabam force-pushed the sort-tags branch 6 times, most recently from eaf2e8e to c68a5ed Compare December 5, 2019 12:36
Signed-off-by: Lev Popov <leo@nabam.net>
Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

Thanks again @nabam. I will merge once automatic checks run and pass.

@everett980 everett980 merged commit 1ee0cf1 into jaegertracing:master Dec 10, 2019
@nabam nabam deleted the sort-tags branch May 4, 2020 09:16
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
Sort span tags in alphabetical order
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sort tags in expanded span view
4 participants