Log Markers on Spans #309

sfriberg · 2019-01-08T04:34:37Z

Signed-off-by: Staffan Friberg sfriberg@kth.se

Which problem is this PR solving?

Currently to see log entries as part of the span you need to expand the details. This PR adds markers in the main Span UI for log entries with a popover showing the log details. Tightly packed log entries are grouped as a single marker until zoomed enough to be rendered separately.

Short description of the changes

The SpanBar class will now create markers for log entries.

Thanks to @tiffon for cleaning up the CSS and switching to Antd Popover instead of Tooltip.

One caveat with the current implementation is that it will on a click in the popover expand the Span details in the main UI instead of anything in table inside the Popover. Any ideas how to get the more expected behavior of expanding the table in the popover instead?

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

yurishkuro · 2019-01-08T05:23:11Z

awesome, resolves #119, a log outstanding issue.

codecov · 2019-01-08T05:24:51Z

Codecov Report

Merging #309 into master will increase coverage by 0.09%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master     #309      +/-   ##
==========================================
+ Coverage   83.04%   83.13%   +0.09%     
==========================================
  Files         142      142              
  Lines        3149     3172      +23     
  Branches      649      654       +5     
==========================================
+ Hits         2615     2637      +22     
- Misses        427      428       +1     
  Partials      107      107

Impacted Files	Coverage Δ
...aceTimelineViewer/SpanDetail/AccordianKeyValues.js	`100% <100%> (ø)`	⬆️
...omponents/TracePage/TraceTimelineViewer/SpanBar.js	`100% <100%> (ø)`	⬆️
...ge/TraceTimelineViewer/SpanDetail/AccordianLogs.js	`100% <100%> (ø)`	⬆️
...onents/TracePage/TraceTimelineViewer/SpanBarRow.js	`86.66% <100%> (+3.33%)`	⬆️
.../components/TracePage/TraceTimelineViewer/utils.js	`100% <100%> (ø)`	⬆️
...cePage/TraceTimelineViewer/VirtualizedTraceView.js	`91.74% <60%> (-1.72%)`	⬇️
...neViewer/TimelineHeaderRow/TimelineViewingLayer.js	`92.59% <0%> (+1.85%)`	⬆️

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 0992eb9...86c0c97. Read the comment docs.

yurishkuro · 2019-01-08T05:26:14Z

Maybe instead of showing all logs in the popup it should only show the log for the current tick, but with all log fields expanded (must be careful with max popup size in this case). It might get tricky / sensitive when there are many ticks next to each other.

Another idea we previously discussed is when the span is expanded, then mousing over the logs highlights the corresponding tick on the span bar.

sfriberg · 2019-01-08T13:49:34Z

Maybe instead of showing all logs in the popup it should only show the log for the current tick, but with all log fields expanded (must be careful with max popup size in this case). It might get tricky / sensitive when there are many ticks next to each other.

It only shows for the "current tick", but I merge ticks close together.
The span in question has 18 logs, and only 3 show for this tick/marker, if you would zoom in further on it it would split in to 3 separate ticks/markers provided they have different timestamps.

Signed-off-by: Staffan Friberg <sfriberg@medallia.com>

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

tiffon

Great PR! Thanks for creating it!

I've added a couple of small comments. Let me know if anything looks awry.

One caveat with the current implementation is that it will on a click in the popover expand the Span details in the main UI instead of anything in table inside the Popover. Any ideas how to get the more expected behavior of expanding the table in the popover instead?

I think it makes sense to keep the popover non-interactive. If the user is interacting with the popover to expand logs, etc., and then accidentally moves the mouse outside the popover, they will lose that state. If the user is interested in details, seems like expanding the row is a nice way to both present the details and keep the log-hints / popover simple.

What do you think?

If we end up going down the road of keeping the popover as-is, so clicking it expands the row instead of interactively expands items in the popover, then what do you think of remove the chevrons (the expand / collapse ">") from the popover, so the log items no longer look interactive?

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanBar.js

tiffon · 2019-01-11T20:28:15Z

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanBar.js

  rpc: {
    viewStart: number,
    viewEnd: number,
    color: string,
  },
  setLongLabel: () => void,
  setShortLabel: () => void,
+  trace: Trace,


Instead of pushing the trace down, maybe it makes sense to push the trace.startTime down since it's only the startTime is needed? Alternatively, the Log data can have a timestampOffset field added when the HTTP trace payload is processed? E.g.

diff --git a/packages/jaeger-ui/src/model/transform-trace-data.js b/packages/jaeger-ui/src/model/transform-trace-data.js index d18ef1e..bc67884 100644 --- a/packages/jaeger-ui/src/model/transform-trace-data.js +++ b/packages/jaeger-ui/src/model/transform-trace-data.js @@ -95,6 +95,8 @@ export default function transfromTraceData(data: TraceData & { spans: SpanWithPr span.relativeStartTime = span.startTime - traceStartTime; span.depth = depth - 1; span.hasChildren = node.children.length > 0; + // eslint-disable-next-line no-param-reassign + span.logs.forEach(log => { log.timestampOffset = log.timestamp - traceStartTime }); span.references.forEach(ref => { const refSpan: ?Span = (spanMap.get(ref.spanID): any); if (refSpan) { diff --git a/packages/jaeger-ui/src/types/trace.js b/packages/jaeger-ui/src/types/trace.js index 5840e46..9c0c5a9 100644 --- a/packages/jaeger-ui/src/types/trace.js +++ b/packages/jaeger-ui/src/types/trace.js @@ -30,6 +30,7 @@ export type Link = { export type Log = { timestamp: number, + timestampOffset: number, fields: Array<KeyValuePair>, };

IMO passing trace.startTime down is preferable as we'd be avoiding baking derived data into the redux state.

What do you think?

Will change to push only the time stamp down, was debating back and forth as well on this and decided on sending the whole trace. Don't know if there is any perf difference with complex objects vs a simple primitive?

Perhaps something to consider for later is if the trace->span->log should backpointers so you could get the span from a log and then the trace from a span.

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/utils.test.js

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/utils.js

sfriberg · 2019-01-11T23:31:54Z

I think it makes sense to keep the popover non-interactive. If the user is interacting with the popover to expand logs, etc., and then accidentally moves the mouse outside the popover, they will lose that state. If the user is interested in details, seems like expanding the row is a nice way to both present the details and keep the log-hints / popover simple.

What do you think?

If we end up going down the road of keeping the popover as-is, so clicking it expands the row instead of interactively expands items in the popover, then what do you think of remove the chevrons (the expand / collapse ">") from the popover, so the log items no longer look interactive?

The main reason to allow expanding is to allow reading longs lines which otherwise would be truncated. I think most people would be fine with losing the state in the popup and common practice in other applications as well. It is just intended for some quick interaction.

I'm fine with either solution, and leave the decision to you. :)

Agree about removing the chevrons if we make it completely non-interactive.

Back to the original question. I can't seem to figure out how to disable that a click in the popover expands the span details. Any clue what needs to change to disable that behavior? (that behavior is non-intuitive and unexpected :) )

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

Signed-off-by: Joe Farro <joef@uber.com>

tiffon · 2019-01-13T21:45:07Z

I found an issue where log markers that are on the far left, right at time=0 (for the current zoom) couldn't be triggered. It was a CSS issue; I pushed an update. Hope you don't mind.

I can't seem to figure out how to disable that a click in the popover expands the span details. Any clue what needs to change to disable that behavior? (that behavior is non-intuitive and unexpected :) )

I think adding event listeners for onClick to the popover and <AccordianLogs> and then calling event.stopPropagation() should do the trick.

The main reason to allow expanding is to allow reading longs lines which otherwise would be truncated. I think most people would be fine with losing the state in the popup and common practice in other applications as well. It is just intended for some quick interaction.

I'm hesitant to have such full-fledged functionality in the logs popovers. Seems like it will get wierd when the height of the popover expands, might be a pain keep the popover positioned, correctly.

I think it might be a nice UX to keep the span detail opening when you click in the popover but also expand the appropriate log entries. I tested it out, here. What do you think?

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

sfriberg · 2019-01-15T00:00:45Z

Was able to disable the clicking in the div. Trying to do it with the AccordionLogs would require several changes to expose the event to the handler methods give to AccordionLogs.

I like the idea, my main concern is that it expands something that easily gets in the way of understanding the flow of the span in the UI. Think there are two ways to improve on that side. Easy one would be to make a second click collapse the details again. A more longterm/experimental way would be to consider if the span details should be shown on the side (static sidebar on the right or left) instead of inline with the spans which breaks visibility of spans as you want to look at details of one of them.

tiffon · 2019-01-15T04:07:01Z

I'll look into the onClick issue.

I see what you mean about the span detail row interrupting the visual flow of span bars. Maybe we should expand the log items in the popover by default?

Re switching to a details panel on the side, I'm absolutely a fan of this. This will allow us to use the same functionality to show span details in the trace graph and trace diffs views. (I thought we had a ticket for moving to a side panel, but I guess not.)

Hover on log markers opens popover Click on log markers toggles span details Log marker popover is not interactive Signed-off-by: Joe Farro <joef@uber.com>

everett980

The types on this function could be a bit more robust:

export function createViewedBoundsFunc({ min, max, viewStart, viewEnd}: {min: number, max: number, viewStart: number, viewEnd: number }): ViewedBoundsFunctionType {
/* ... */
}

But that's not critical. Otherwise looks good 👍

sfriberg · 2019-02-19T17:18:39Z

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

              label={`${formatDuration(log.timestamp - timestamp)}`}
-              onToggle={() => onItemToggle(log)}
+              linksGetter={linksGetter}
+              onToggle={interactive && onItemToggle ? () => onItemToggle(log) : null}


No sure what the preference/recommendation is.

Would it make sense to use undefined instead of null as it is defined as "onToggle?" in the properties?

More of a general question about using null vs undefined than anything.

As a practice, I generally use null, which differentiates something set to null from something that's not there. E.g.

const o = {}; console.log(o.notThere === null); // false // for convenience, `==` is easier than `value === null || value === undefined` console.log(o.notThere == null); // true

CHANGELOG.md

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanBarRow.js

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

tiffon · 2019-02-21T19:59:50Z

@everett980 Thanks for the catch re the flow types. Turns out @ flow was missing form the file!

Signed-off-by: Joe Farro <joef@uber.com>

tiffon · 2019-02-21T22:28:08Z

@sfriberg The log markers are merged! 🎉 🎉 🎉

Thanks so much for putting this together!

* Log Markers on Spans Signed-off-by: Staffan Friberg <sfriberg@kth.se> * Make tests pass with empty logs for now Signed-off-by: Staffan Friberg <sfriberg@kth.se> * Simple test that log markers are created Signed-off-by: Staffan Friberg <sfriberg@medallia.com> * Push trace separately all they way down to SpanBar Signed-off-by: Staffan Friberg <sfriberg@kth.se> * Send Span all the way to SpanBar instead of Logs Signed-off-by: Staffan Friberg <sfriberg@kth.se> * PR comments Signed-off-by: Staffan Friberg <sfriberg@kth.se> * Update changelog for PR 309 Signed-off-by: Joe Farro <joef@uber.com> * CSS fix for logMarker hover at t0 Signed-off-by: Joe Farro <joef@uber.com> * Disable clicking Signed-off-by: Staffan Friberg <sfriberg@kth.se> * Tweaks to UX for span log markers Hover on log markers opens popover Click on log markers toggles span details Log marker popover is not interactive Signed-off-by: Joe Farro <joef@uber.com> * Fix changelog, missing "@ flow" and log-marker spacing Signed-off-by: Joe Farro <joef@uber.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

sfriberg added 2 commits January 7, 2019 23:12

Log Markers on Spans

2052309

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

Make tests pass with empty logs for now

0478c8f

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

sfriberg force-pushed the logMarkers branch from 415995a to 0478c8f Compare January 8, 2019 05:15

sfriberg added 3 commits January 8, 2019 09:42

Simple test that log markers are created

0a99c0a

Signed-off-by: Staffan Friberg <sfriberg@medallia.com>

Push trace separately all they way down to SpanBar

a8749ee

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

Send Span all the way to SpanBar instead of Logs

9ee80a5

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

tiffon requested a review from everett980 January 11, 2019 08:39

Merge branch 'master' into logMarkers

2c6a2f5

ghost assigned tiffon Jan 11, 2019

ghost added the review label Jan 11, 2019

tiffon suggested changes Jan 11, 2019

View reviewed changes

everett980 reviewed Jan 11, 2019

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/utils.test.js Outdated Show resolved Hide resolved

everett980 reviewed Jan 11, 2019

View reviewed changes

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/utils.js Outdated Show resolved Hide resolved

sfriberg and others added 4 commits January 11, 2019 17:35

PR comments

dd98cc3

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

Merge branch 'master' into logMarkers

78c3a2f

Update changelog for PR 309

bdceca6

Signed-off-by: Joe Farro <joef@uber.com>

CSS fix for logMarker hover at t0

c5432e1

Signed-off-by: Joe Farro <joef@uber.com>

Disable clicking

f79ad53

Signed-off-by: Staffan Friberg <sfriberg@kth.se>

sfriberg force-pushed the logMarkers branch from 262ce0f to f79ad53 Compare January 14, 2019 23:56

sfriberg and others added 3 commits January 22, 2019 15:28

Merge branch 'master' into logMarkers

e421c1a

Tweaks to UX for span log markers

c3e3601

Hover on log markers opens popover Click on log markers toggles span details Log marker popover is not interactive Signed-off-by: Joe Farro <joef@uber.com>

Merge branch 'master' into logMarkers

bc21fdb

everett980 approved these changes Feb 19, 2019

View reviewed changes

sfriberg commented Feb 19, 2019

View reviewed changes

yurishkuro reviewed Feb 19, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanBarRow.js Show resolved Hide resolved

packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/SpanDetail/AccordianLogs.js Show resolved Hide resolved

Merge branch 'master' into logMarkers

bc01d88

Fix changelog, missing "@ flow" and log-marker spacing

86c0c97

Signed-off-by: Joe Farro <joef@uber.com>

tiffon approved these changes Feb 21, 2019

View reviewed changes

tiffon merged commit 2abe94e into jaegertracing:master Feb 21, 2019

ghost removed the review label Feb 21, 2019

everett980 mentioned this pull request Sep 12, 2019

Proposal: Visualization of log timestamps in timeline #119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log Markers on Spans #309

Log Markers on Spans #309

sfriberg commented Jan 8, 2019

yurishkuro commented Jan 8, 2019 •

edited

Loading

codecov bot commented Jan 8, 2019 •

edited

Loading

yurishkuro commented Jan 8, 2019

sfriberg commented Jan 8, 2019 •

edited

Loading

tiffon left a comment

tiffon Jan 11, 2019

sfriberg Jan 11, 2019

sfriberg commented Jan 11, 2019 •

edited

Loading

tiffon commented Jan 13, 2019

sfriberg commented Jan 15, 2019

tiffon commented Jan 15, 2019 •

edited

Loading

everett980 left a comment

sfriberg Feb 19, 2019

tiffon Feb 21, 2019

tiffon commented Feb 21, 2019

tiffon commented Feb 21, 2019

Log Markers on Spans #309

Log Markers on Spans #309

Conversation

sfriberg commented Jan 8, 2019

Which problem is this PR solving?

Short description of the changes

yurishkuro commented Jan 8, 2019 • edited Loading

codecov bot commented Jan 8, 2019 • edited Loading

Codecov Report

yurishkuro commented Jan 8, 2019

sfriberg commented Jan 8, 2019 • edited Loading

tiffon left a comment

Choose a reason for hiding this comment

tiffon Jan 11, 2019

Choose a reason for hiding this comment

sfriberg Jan 11, 2019

Choose a reason for hiding this comment

sfriberg commented Jan 11, 2019 • edited Loading

tiffon commented Jan 13, 2019

sfriberg commented Jan 15, 2019

tiffon commented Jan 15, 2019 • edited Loading

everett980 left a comment

Choose a reason for hiding this comment

sfriberg Feb 19, 2019

Choose a reason for hiding this comment

tiffon Feb 21, 2019

Choose a reason for hiding this comment

tiffon commented Feb 21, 2019

tiffon commented Feb 21, 2019

@sfriberg The log markers are merged! 🎉 🎉 🎉

yurishkuro commented Jan 8, 2019 •

edited

Loading

codecov bot commented Jan 8, 2019 •

edited

Loading

sfriberg commented Jan 8, 2019 •

edited

Loading

sfriberg commented Jan 11, 2019 •

edited

Loading

tiffon commented Jan 15, 2019 •

edited

Loading