Skip to content

fix(trace) account for head or tail end of icons #72884

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

Merged
merged 6 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions static/app/views/performance/newTraceDetails/trace.tsx
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
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
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
Loading