Add tooltip when hovering Critical Path

[GLVSKiriti]

Which problem is this PR solving?

• Related to [feature] Visualize Critical Path of a trace #1582

Description of the changes

• Added Tooltip which is visible on hovering the critical path

@yurishkuro

[GLVSKiriti]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
...mponents/TracePage/TraceTimelineViewer/SpanBar.tsx	100.00% <ø> (ø)
...ePage/TraceTimelineViewer/VirtualizedTraceView.tsx	97.67% <100.00%> (+0.72%)	⬆️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[yurishkuro]

Thanks, looks good. I suggested a change to the wording to be a bit more explicit. An even more advanced form would be to use display similar to logs where we show a full list of CP segments in the span with some of their stats, like start, end, % of total CP. But we can start with just static popup.

There is one issue currently, though. When the span also has log ticks, it's almost impossible to get the logs popup to show if there's also a CP segment in the same timeframe. Somehow we need to make sure that the logs tooltip takes priority over CP. Perhaps this could be achieved with z-order, I am not sure.

cc @anshgoyalevil

[anshgoyalevil]

I just gave it a go, and yes, the zIndex solution would work here.

<div
  data-testid="SpanBar--logMarker"
  className="SpanBar--logMarker"
  style={{ left: positionKey, zIndex: 3}}
/>

(I tried with the value 3 (zIndex). Anything lower than that wouldn't work.)

A design suggestion: Can we change the color of CP on mouse hover? It would make more sense to the user that ToolTip is related the CP, and not the whole span.

[anshgoyalevil]

@yurishkuro
How about an addition? Color change on hover

[yurishkuro]

How about an addition? Color change on hover

Not a bad idea, as long as the color is distinct from the span color palette. E.g., don't we already use brown for spans?

I also think it would be nice for the log ticks to enlarge when hovered (like using a bolder line), and maybe have a slightly larger clickable bounding box. Right now it takes very fine pointing to get a popup from a log tick.

[anshgoyalevil]

don't we already use brown for spans?

We already use maybe. Not sure though, I chose brown just for example

I also think it would be nice for the log ticks to enlarge when hovered (like using a bolder line), and maybe have a slightly larger clickable bounding box. Right now it takes very fine pointing to get a popup from a log tick.

As far as I understand, isn't the size of that point so thin, to accommodate all ticks in a continuous timeframe of milliseconds?

Another POV, we can increase the groupBy roundoff percentage on L93 to accommodate more logs under a single bold tick

[yurishkuro]

so thin, to accommodate all ticks in a continuous timeframe of milliseconds

Yes, but when you have too many logs clustered together no matter what you do they will not be distinguishable without zooming in on the timeline. For comparison, Zipkin UI used to use oval pills to represent event (much thicker than our ticks):

[anshgoyalevil]

Ah yes, I forgot that the end users can zoom in on the timeline.

[GLVSKiriti]

Not a bad idea, as long as the color is distinct from the span color palette. E.g., don't we already use brown for spans?

What about color White?

[yurishkuro]

We have many light colors like yellow in the same pic, I'm not sure white will be contrasting enough on those. May we could do animation of white spot running left to right once?

[GLVSKiriti]

May we could do animation of white spot running left to right once?

We can adjust the spot width and time

But if child spans are collapsed then it would be like this

Is this ok?

[yurishkuro]

The first s/s is awesome!

When children are collapsed, wouldn't the CP segments have identical edge timestamps, like [t1, t2], [t2, t3], ...? If so, why don't we merge them? It would help this running spot, and in general it's meaningless to have adjacent CP segments without a gap between them.

[GLVSKiriti]

When children are collapsed, wouldn't the CP segments have identical edge timestamps, like [t1, t2], [t2, t3], ...? If so, why don't we merge them? It would help this running spot, and in general it's meaningless to have adjacent CP segments without a gap between them.

Yep I merged the CP segments and the result would be like this!

I also think it would be nice for the log ticks to enlarge when hovered (like using a bolder line),

And I also increased size of log ticks !

[yurishkuro]

We should really just add a helper method to the Trace object: trace.findSpan(spanID)

[yurishkuro]

thanks!

[GLVSKiriti]

what about adding a helper method trace.findSpan(spanID)?Should I do it in seperate PR

[yurishkuro]

Well, to be honest, and I raised this during the project, this whole searching should go away. We build a proper tree in one place, we should just keep it as part of the trace and always navigate it, instead of doing the same work over and over. All these searches are O(N).

[bobrik]

This might be pretty taxing on CPU usage. We're seeing very slow tag expansion in spans in large traces.

Here's me clicking to expand the tags and then to collapse them back:

[yurishkuro]

@bobrik please open a new ticket. You can disable critical path for all traces via config, but there's certainly room for perf optimization.

[bobrik]

I filed #2179 for this.