Deduplicate tags for spans #375

rubenvp8510 · 2019-05-03T06:11:47Z

Signed-off-by: Ruben Vargas [email protected]

Which problem is this PR solving?

This solves issue: See "error" tag in UI multiple time jaeger#1470, at least from the UI perspective, not sure if this is the best way.

Short description of the changes

I check for duplicated tags, and remove it (I left only one of them)
Also has a boolean to indicate that span has duplicated tags so we can show a warning or something

Screenshot of warning:

codecov · 2019-05-03T17:53:50Z

Codecov Report

Merging #375 into master will increase coverage by <.01%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master     #375      +/-   ##
==========================================
+ Coverage   88.71%   88.72%   +<.01%     
==========================================
  Files         157      159       +2     
  Lines        3518     3556      +38     
  Branches      802      811       +9     
==========================================
+ Hits         3121     3155      +34     
- Misses        362      365       +3     
- Partials       35       36       +1

Impacted Files	Coverage Δ
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx	`92.37% <ø> (ø)`	⬆️
...ts/TracePage/TraceTimelineViewer/SpanDetailRow.tsx	`100% <ø> (ø)`	⬆️
...age/TraceTimelineViewer/SpanDetail/DetailState.tsx	`95.83% <100%> (+0.83%)`	⬆️
...TracePage/TraceTimelineViewer/SpanDetail/index.tsx	`100% <100%> (ø)`	⬆️
...cePage/TraceTimelineViewer/SpanDetail/TextList.tsx	`100% <100%> (ø)`
.../components/TracePage/TraceTimelineViewer/duck.tsx	`98.4% <100%> (+0.05%)`	⬆️
...kages/jaeger-ui/src/model/transform-trace-data.tsx	`84.61% <91.66%> (+1.59%)`	⬆️
...e/TraceTimelineViewer/SpanDetail/AccordianText.tsx	`92.3% <92.3%> (ø)`
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	`88.13% <0%> (-3.39%)`	⬇️
... and 1 more

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 bfd32be...6e67ca2. Read the comment docs.

yurishkuro · 2019-05-03T18:53:07Z

I like the warning icon, but we have a built-in place for warnings in the spans (below), which may contain more than one warning. So it would be better to display them as a list. Maybe we can add another expandable item below logs called Warnings({count}), which may be prefixed with the warning icon for better visuals, and not show up at all if span has no warnings.

https://github.com/jaegertracing/jaeger/blob/3fff10206109b54c9d8a42cb1347336836a9b0ea/model/model.pb.go#L359

rubenvp8510 · 2019-05-03T19:48:14Z

Ahh! I haven't noticed that field in span object, in that case it makes more sense to put warnings there. I'll do the modifications and update this PR, with new screenshots also.

rubenvp8510 · 2019-05-04T18:10:18Z

@yurishkuro WDYT?

yurishkuro · 2019-05-12T20:08:02Z

@rubenvp8510 s/s looks good to me. I assume the Warnings will not show up if there are no warnings?

@tiffon is this ready to merge?

rubenvp8510 · 2019-05-12T20:23:08Z

@yurishkuro yes, the warning accordion does not show up if there are no warning on the span

tiffon

This looks great! Thank you!

I left a few comments. LMK if any look awry.

I experimented a bit with the styling, to see what it would look like using color instead of the icon:

What do you think?

Thank you again, and apologies for the delay in reviewing!

tiffon · 2019-05-14T16:06:10Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianLogs.css

@@ -24,6 +24,7 @@ limitations under the License.
  color: inherit;
  display: block;
  padding: 0.25rem 0.5rem;
+  padding-left: 0;


The left padding allows the log section chevron to align with the chevrons from the individual log messages. Setting padding-left: 0 causes a misalignment.

Before:

tiffon · 2019-05-14T16:12:09Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianLogs.tsx

@@ -56,7 +56,7 @@ export default function AccordianLogs(props: AccordianLogsProps) {
  }

  return (
-    <div className="AccordianLogs">
+    <div className="AccordianLogs ub-mb1">


Instead of adding a util class please add the style to the .AccordianLogs class. I try to only use util styles when the element is not already being styled via a CSS class specific to that element.

u-simple-scrollbars is the main exception.

tiffon · 2019-05-15T02:26:09Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/TextList.css

+  padding: 0.25rem 0.5rem;
+}
+
+.TextList-row:nth-child(2n) > td {


Looks like there is a missing second -

.TextList--row:nth-child(2n)

tiffon · 2019-05-15T02:59:44Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/TextList.css

+  vertical-align: baseline;
+}
+
+.TextList--row > td {


I think this is a dupe of the style on line 37 and can be removed?

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/TextList.css

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/TextList.tsx

packages/jaeger-ui/src/model/transform-trace-data.tsx

packages/jaeger-ui/src/types/trace.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/duck.tsx

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianText.css

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 · 2019-05-15T23:24:53Z

Hi @tiffon I did all requested modifications, also I modified the style according to your suggestion, I think looks better and more "inline" with the actual UI style.

I attached some screen-shots of the latest modifications.

Could you please review again? Thank you a lot!

Signed-off-by: Joe Farro <[email protected]>

tiffon

Looks great! Awesome.

I pushed a couple of styling tweaks, mostly adding a bottom border to the warnings header, when it's open, setting its content to a light grey background, and adding a headerClassName prop so the CSS styles aren't coupled to the AccordingText implementation.

Let me know if anything about those changes look awry...?

rubenvp8510 · 2019-05-16T02:53:40Z

Looks very good! Thanks for the improvement!

tiffon · 2019-05-16T07:31:04Z

Hey, awesome, done! 👍

Deduplicate tags for spans Signed-off-by: vvvprabhakar <[email protected]>

rubenvp8510 force-pushed the deduplicate-tags branch from ce20fcb to 7d6621a Compare May 3, 2019 06:24

rubenvp8510 force-pushed the deduplicate-tags branch from 3b4188d to 48d9a6c Compare May 3, 2019 17:54

rubenvp8510 changed the title ~~[WIP] Deduplicate tags for spans~~ Deduplicate tags for spans May 3, 2019

rubenvp8510 force-pushed the deduplicate-tags branch 2 times, most recently from b008e31 to dcfa684 Compare May 3, 2019 18:27

rubenvp8510 force-pushed the deduplicate-tags branch from 28d047f to 7469c7a Compare May 4, 2019 18:09

rubenvp8510 force-pushed the deduplicate-tags branch 11 times, most recently from ac17f3f to a4bca89 Compare May 5, 2019 15:03

objectiser mentioned this pull request May 13, 2019

Tracking release 1.12.0 jaegertracing/jaeger#1528

Closed

rubenvp8510 force-pushed the deduplicate-tags branch from a4bca89 to 2779121 Compare May 14, 2019 21:00

tiffon suggested changes May 15, 2019

View reviewed changes

Deduplicate tags for spans

242087e

Signed-off-by: Ruben Vargas <[email protected]>

rubenvp8510 force-pushed the deduplicate-tags branch from 2779121 to 242087e Compare May 15, 2019 23:08

Styling tweaks to warnings in span details

25c9082

Signed-off-by: Joe Farro <[email protected]>

ghost assigned tiffon May 16, 2019

ghost added the review label May 16, 2019

tiffon added 2 commits May 15, 2019 22:31

Fix span detail warnings CSS typo

90fe66f

Signed-off-by: Joe Farro <[email protected]>

Fix span detail warnings CSS typo

6e67ca2

Signed-off-by: Joe Farro <[email protected]>

tiffon approved these changes May 16, 2019

View reviewed changes

tiffon merged commit 126f42c into jaegertracing:master May 16, 2019

ghost removed the review label May 16, 2019

vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021

Merge pull request jaegertracing#375 from rubenvp8510/deduplicate-tags

e828f0d

Deduplicate tags for spans Signed-off-by: vvvprabhakar <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate tags for spans #375

Deduplicate tags for spans #375

rubenvp8510 commented May 3, 2019 •

edited

Loading

codecov bot commented May 3, 2019 •

edited

Loading

yurishkuro commented May 3, 2019

rubenvp8510 commented May 3, 2019

rubenvp8510 commented May 4, 2019

yurishkuro commented May 12, 2019

rubenvp8510 commented May 12, 2019

tiffon left a comment

tiffon May 14, 2019

tiffon May 14, 2019

tiffon May 15, 2019

tiffon May 15, 2019

rubenvp8510 commented May 15, 2019 •

edited

Loading

tiffon left a comment

rubenvp8510 commented May 16, 2019

tiffon commented May 16, 2019

Deduplicate tags for spans #375

Deduplicate tags for spans #375

Conversation

rubenvp8510 commented May 3, 2019 • edited Loading

Which problem is this PR solving?

Short description of the changes

codecov bot commented May 3, 2019 • edited Loading

Codecov Report

yurishkuro commented May 3, 2019

rubenvp8510 commented May 3, 2019

rubenvp8510 commented May 4, 2019

yurishkuro commented May 12, 2019

rubenvp8510 commented May 12, 2019

tiffon left a comment

Choose a reason for hiding this comment

tiffon May 14, 2019

Choose a reason for hiding this comment

tiffon May 14, 2019

Choose a reason for hiding this comment

tiffon May 15, 2019

Choose a reason for hiding this comment

tiffon May 15, 2019

Choose a reason for hiding this comment

rubenvp8510 commented May 15, 2019 • edited Loading

tiffon left a comment

Choose a reason for hiding this comment

rubenvp8510 commented May 16, 2019

tiffon commented May 16, 2019

rubenvp8510 commented May 3, 2019 •

edited

Loading

codecov bot commented May 3, 2019 •

edited

Loading

rubenvp8510 commented May 15, 2019 •

edited

Loading