Skip to content

Commit

Permalink
fix(trace) account for head or tail end of icons (#72884)
Browse files Browse the repository at this point in the history 
Span bars sometimes have icons at the head or tail end, which means they
can overlap with the duration bars and cause visual issues. This PR
computes the span bar space according to the min and max timestamps so
that the duration labels no longer overlap.

Before:


https://github.com/getsentry/sentry/assets/9317857/d8ff6efc-7145-49ff-a7e1-4ab6a8c21f0a

After:


https://github.com/getsentry/sentry/assets/9317857/fd23c752-e5b3-4623-885f-e51460074a75

---------

Co-authored-by: Tony Xiao <txiao@sentry.io>
Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
  • Loading branch information
@JonasBa @Zylphrex @getsantry
3 people authored Jun 18, 2024
1 parent 6703e65 commit 7b264ef
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 62 deletions.
32 changes: 21 additions & 11 deletions static/app/views/performance/newTraceDetails/trace.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1433,17 +1433,16 @@ function BackgroundPatterns(props: BackgroundPatternsProps) {
);

return (
<Fragment key={i}>
<div
className="TracePatternContainer"
style={{
left: left * 100 + '%',
width: (1 - left) * 100 + '%',
}}
>
<div className="TracePattern performance_issue" />
</div>
</Fragment>
<div
key={i}
className="TracePatternContainer"
style={{
left: left * 100 + '%',
width: (1 - left) * 100 + '%',
}}
>
<div className="TracePattern performance_issue" />
</div>
);
})}
</Fragment>
Expand Down Expand Up @@ -2247,6 +2246,17 @@ const TraceStylingWrapper = styled('div')`
fill: ${p => p.theme.white};
}
}
&.error {
color: ${p => p.theme.red300};
.TraceChildrenCountWrapper {
button {
color: ${p => p.theme.white};
background-color: ${p => p.theme.red300};
}
}
}
}
}
Expand Down
3 changes: 3 additions & 0 deletions static/app/views/performance/newTraceDetails/traceModels/traceTree.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,9 @@ export function makeTraceNodeBarColor(
return pickBarColor(node.value.op);
}
if (isAutogroupedNode(node)) {
if (node.errors.size > 0) {
return theme.red300;
}
return theme.blue300;
}
if (isMissingInstrumentationNode(node)) {
Expand Down
119 changes: 68 additions & 51 deletions static/app/views/performance/newTraceDetails/traceRenderers/virtualizedViewManager.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1170,64 +1170,37 @@ export class VirtualizedViewManager {
span_space: [number, number],
text: string
): [number, number] {
const text_left = span_space[0] > this.to_origin + this.trace_space.width * 0.8;
const width = this.text_measurer.measure(text);

const has_profiles = node && node.profiles.length > 0;
const has_error_icons =
node &&
(node.profiles.length > 0 ||
node.errors.size > 0 ||
node.performance_issues.size > 0);
const TEXT_PADDING = 2;

const has_icons = has_profiles || has_error_icons;
const icon_width_config_space = (18 * this.span_to_px[0]) / 2;
const text_anchor_left =
span_space[0] > this.to_origin + this.trace_space.width * 0.8;
const text_width = this.text_measurer.measure(text);

const node_width = span_space[1] / this.span_to_px[0];
const TEXT_PADDING = 2;
// This is inaccurate in the case of left anchored text. In order to determine a true overlap, we would need to compute
// the distance between the min timestamp of an icon and beginning of the span. Once we determine the distance, we can compute
// the width and see if there is an actual overlap. Since this is a rare case which only happens in the case where we anchor the text
// to the left (20% of the time) and the node may have many errors, this could be computationally expensive to do on every frame.
// We'll live with the inaccuracy for now as it is purely visual and just make sure to handle a single error case as it will be easy
// to determine if there is an overlap.
const TEXT_PADDING_LEFT = text_left && has_icons ? 10 : TEXT_PADDING;

const TEXT_PADDING_RIGHT =
!text_left && has_icons
? node_width < 10
? // If the node is too small, we need to make sure the text is anchored to the right edge of the icon.
// We take the distance from the right edge of the node to the right edge of the icon and subtract it from
// the base width (10) and the base padding when (expanded) to get the correct padding. If we take only 10px
// as our padding, the text can be anchored directly to the right edge of our icon - we want to preserve
// a min padding of 2px.
12 - node_width
: TEXT_PADDING
: TEXT_PADDING;
const timestamps = getIconTimestamps(node, span_space, icon_width_config_space);
const text_left = Math.min(span_space[0], timestamps[0]);
const text_right = Math.max(span_space[0] + span_space[1], timestamps[1]);

// precompute all anchor points aot, so we make the control flow more readable.
// this wastes some cycles, but it's not a big deal as computers go brrrr when it comes to simple arithmetic.
/// |---| text
const right_outside =
this.computeTransformXFromTimestamp(span_space[0] + span_space[1]) +
TEXT_PADDING_RIGHT;
/// text |---|
const left_outside =
this.computeTransformXFromTimestamp(span_space[0]) - TEXT_PADDING_LEFT - width;

// | text|
const right_outside = this.computeTransformXFromTimestamp(text_right) + TEXT_PADDING;
// |---text|
const right_inside =
this.computeTransformXFromTimestamp(span_space[0] + span_space[1]) -
width -
text_width -
TEXT_PADDING;
// |text |
// |text---|
const left_inside = this.computeTransformXFromTimestamp(span_space[0]) + TEXT_PADDING;
/// text |---|
const left_outside =
this.computeTransformXFromTimestamp(text_left) - TEXT_PADDING - text_width;

// Right edge of the window (when span extends beyond the view)
const window_right =
this.computeTransformXFromTimestamp(
this.to_origin + this.trace_view.left + this.trace_view.width
) -
width -
text_width -
TEXT_PADDING;
const window_left =
this.computeTransformXFromTimestamp(this.to_origin + this.trace_view.left) +
Expand All @@ -1244,22 +1217,22 @@ export class VirtualizedViewManager {

// Span is completely outside of the view on the left side
if (span_right < this.trace_view.x) {
return text_left ? [1, right_inside] : [0, right_outside];
return text_anchor_left ? [1, right_inside] : [0, right_outside];
}

// Span is completely outside of the view on the right side
if (span_left > this.trace_view.right) {
return text_left ? [0, left_outside] : [1, left_inside];
return text_anchor_left ? [0, left_outside] : [1, left_inside];
}

// Span "spans" the entire view
if (span_left <= this.trace_view.x && span_right >= this.trace_view.right) {
return text_left ? [1, window_left] : [1, window_right];
return text_anchor_left ? [1, window_left] : [1, window_right];
}

const full_span_px_width = span_space[1] / this.span_to_px[0];

if (text_left) {
if (text_anchor_left) {
// While we have space on the left, place the text there
if (space_left > 0) {
return [0, left_outside];
Expand All @@ -1270,7 +1243,7 @@ export class VirtualizedViewManager {

// If the text fits inside the visible portion of the span, anchor it to the left
// side of the window so that it is visible while the user pans the view
if (visible_width - TEXT_PADDING >= width) {
if (visible_width - TEXT_PADDING >= text_width) {
return [1, window_left];
}

Expand All @@ -1292,22 +1265,22 @@ export class VirtualizedViewManager {
// origin and check if it fits into the distance of space right edge - span right edge. In practice
// however, it seems that a magical number works just fine.
span_right > this.trace_space.right * 0.9 &&
space_right / this.span_to_px[0] < width
space_right / this.span_to_px[0] < text_width
) {
return [1, right_inside];
}
return [0, right_outside];
}

// If text fits inside the span
if (full_span_px_width > width) {
if (full_span_px_width > text_width) {
const distance = span_right - this.trace_view.right;
const visible_width =
(span_space[1] - distance) / this.span_to_px[0] - TEXT_PADDING;

// If the text fits inside the visible portion of the span, anchor it to the right
// side of the window so that it is visible while the user pans the view
if (visible_width - TEXT_PADDING >= width) {
if (visible_width - TEXT_PADDING >= text_width) {
return [1, window_right];
}

Expand Down Expand Up @@ -1704,6 +1677,50 @@ export class VirtualizedViewManager {
}
}

// Computes a min and max icon timestamp. This effectively extends or reduces the hitbox
// of the span to include the icon. We need this because when the icon is close to the edge
// it can extend it and cause overlaps with duration labels
function getIconTimestamps(
node: TraceTreeNode<any>,
span_space: [number, number],
icon_width: number
) {
let min_icon_timestamp = span_space[0];
let max_icon_timestamp = span_space[0] + span_space[1];

if (!node.errors.size && !node.performance_issues.size) {
return [min_icon_timestamp, max_icon_timestamp];
}

for (const issue of node.performance_issues) {
// Perf issues render icons at the start timestamp
if (typeof issue.start === 'number') {
min_icon_timestamp = Math.min(min_icon_timestamp, issue.start * 1e3 - icon_width);
max_icon_timestamp = Math.max(max_icon_timestamp, issue.start * 1e3 + icon_width);
}
}

for (const err of node.errors) {
if (typeof err.timestamp === 'number') {
min_icon_timestamp = Math.min(min_icon_timestamp, err.timestamp * 1e3 - icon_width);
max_icon_timestamp = Math.max(max_icon_timestamp, err.timestamp * 1e3 + icon_width);
}
}

min_icon_timestamp = clamp(
min_icon_timestamp,
span_space[0] - icon_width,
span_space[0] + span_space[1] + icon_width
);
max_icon_timestamp = clamp(
max_icon_timestamp,
span_space[0] - icon_width,
span_space[0] + span_space[1] + icon_width
);

return [min_icon_timestamp, max_icon_timestamp];
}

// Jest does not implement scroll updates, however since we have the
// middleware to handle scroll updates, we can dispatch a scroll event ourselves
function dispatchJestScrollUpdate(container: HTMLElement) {
Expand Down

0 comments on commit 7b264ef

Please sign in to comment.