-
Notifications
You must be signed in to change notification settings - Fork 153
improve llm spans view #1061
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
base: dev
Are you sure you want to change the base?
improve llm spans view #1061
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 1927ffe in 2 minutes and 18 seconds. Click for details.
- Reviewed
194lines of code in4files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/shared/traces/trace-view.tsx:87
- Draft comment:
Extra newline removed; stylistic cleanup only. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. frontend/components/traces/trace-view/span-card.tsx:89
- Draft comment:
Hover logic now toggles the displayed LLM span name; verify this UX change is intended. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. frontend/components/traces/trace-view/utils.ts:255
- Draft comment:
Falsely checking for cost or totalTokens may skip valid 0 values; consider explicit undefined/null checks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment identifies a potential issue with the falsy check on line 255. Let me analyze:costis a string from.toFixed(3), so even "0.000" would be truthy. However,totalTokensis a number, and if it's 0 (both input and output tokens are 0), the check!totalTokenswould be true and return null. This could be a legitimate case - an LLM call with 0 tokens might be valid (though unusual). However, there's also a potential bug on line 251: ifspan.attributes["gen_ai.usage.cost"]is undefined or null, calling.toFixed(3)would throw an error. The code doesn't handle this case. So the comment is partially correct about the totalTokens check, but misses the more critical issue that the code would crash if the cost attribute is missing. The comment focuses on the falsy check but doesn't mention that line 251 would crash if the cost attribute is undefined/null (calling .toFixed on undefined). Also, I need to consider whether 0 tokens is actually a valid case in practice - it might be that 0 tokens should indeed return null as it indicates no actual LLM usage. Without more context about the business logic, I can't be certain this is a bug. While the comment identifies a potential issue with the totalTokens check treating 0 as falsy, there's a more critical bug it misses (the .toFixed crash). Additionally, whether 0 tokens is a "valid" case depends on business logic I don't have access to. The comment is technically correct about the falsy check behavior, but without knowing if 0 is a valid business case, I can't confirm this requires a fix. The comment is technically correct that the falsy check would treat 0 totalTokens as invalid, but it's unclear if this is actually a bug or intended behavior. More critically, the code has a different bug (line 251 would crash if cost attribute is missing) that the comment doesn't address. Given the uncertainty and the rule to require STRONG EVIDENCE, I should delete this comment.
Workflow ID: wflow_fb2I0Fwi1Mmad4oK
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| export const getLLMMetrics = (span:TraceViewSpan)=>{ | ||
| if (span.spanType !== "LLM") return null; | ||
|
|
||
| const cost = span.attributes["gen_ai.usage.cost"].toFixed(3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure gen_ai.usage.cost exists and is a number before calling toFixed(3) to avoid runtime errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed ba36867 in 2 minutes and 22 seconds. Click for details.
- Reviewed
94lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/span-card.tsx:106
- Draft comment:
Consider extracting the LLM metrics display into a shared component to avoid duplicating similar code across span-card and timeline-element. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. frontend/components/traces/trace-view/timeline-element.tsx:77
- Draft comment:
For better accessibility, add aria-hidden='true' to decorative icons (Coins, CircleDollarSign) so that screen readers skip them. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_mgBJ2YCI9kJ0kkrk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| <Coins className="min-w-3" size={12} /> | ||
| {llmMetrics.totalTokens} | ||
| </div> | ||
| <div className={'text-white/70 inline-flex items-center gap-1 ml-4'} style={{marginLeft: 4}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second metrics block uses both a Tailwind class (ml-4) and an inline style (marginLeft: 4). This redundancy may lead to inconsistent spacing. Consider using one consistent approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed b201b15 in 1 minute and 8 seconds. Click for details.
- Reviewed
23lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/utils.ts:251
- Draft comment:
Good improvement: explicit null checks prevent misinterpreting falsy values (e.g., 0) as missing. Consider ensuring that cost, inputTokens, and outputTokens are numbers to avoid potential runtime errors (e.g., when calling toFixed and adding). - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_ebaWIqJ3QmP6nXzn
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 5352847 in 28 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/utils.ts:243
- Draft comment:
Good enhancement: now only returns the model name if it exists; otherwise, it gracefully falls back to the original span name. Consider whether empty strings are possible and if an explicit check (e.g. modelName !== '') is needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_mkbKD41g4s8kCZ6k
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed fe7fa54 in 1 minute and 50 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/timeline-element.tsx:73
- Draft comment:
The token badge now uses with class 'min-w-1' instead of 'min-w-3'. Verify if the reduced minimum width aligns with design requirements. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. frontend/components/traces/trace-view/timeline-element.tsx:77
- Draft comment:
Consider using Tailwind utility classes for margin (e.g., ml-*) instead of inline style (marginLeft: 4) for consistency. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_bKE5SMlG01Nuru6W
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| const textContent = ( | ||
| <> | ||
| {span.span.name}{" "} | ||
| <div className={'flex items-center gap-1.5'}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a <div> (block-level) for textContent can cause invalid HTML when nested inside a <span> (from commonProps). Prefer an inline element such as <span> with flex classes (e.g., inline-flex) to maintain valid markup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 10df28d in 2 minutes and 4 seconds. Click for details.
- Reviewed
101lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/span-card.tsx:108
- Draft comment:
Removed badge spacing: The original ml-2 and inline margin style on the token and cost badge divs have been removed. Please confirm that reducing or removing the horizontal spacing was intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. frontend/components/traces/trace-view/timeline-element.tsx:151
- Draft comment:
Duplicate outside text element and ref usage: When textPosition is 'outside', an explicit (lines ~151–160) and the memoized spanTextElement (rendered again on line ~190) are both rendered, both using the same ref. This may lead to duplicate rendering and potential ref conflicts. Please confirm the intended behavior. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_I2XiE7Oc9YJ26gfY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 6a21c39 in 2 minutes and 20 seconds. Click for details.
- Reviewed
169lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/trace-view-store-utils.ts:88
- Draft comment:
Consider memoizing the aggregated metrics to avoid redundant recursive computations on large span trees. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about a performance optimization suggestion. Looking at the implementation, the code does callaggregateMetricsFromChildrenfor each span in the array (line 145), and this function recursively processes children. However, the results are stored inspansWithMetricsand then used to rebuild the child/parent maps. The key question is: does the current implementation actually have redundant computations? Looking at lines 144-147, each span gets its metrics computed once and stored. The recursive calls withinaggregateMetricsFromChildrentraverse the tree, but each span's metrics are only computed once in the map operation. The comment might be technically correct that memoization could help, but it's not clear there's actually a performance problem here that needs solving. The comment is speculative about performance without evidence of an actual issue. The comment might be valid if there are indeed redundant computations happening, but I need to verify this more carefully. The code structure suggests that each span's metrics are computed once during the map operation, so memoization might not provide significant benefit. Also, this is a code quality/performance suggestion without clear evidence of a problem, which might fall under "speculative" comments. While the comment is about code quality and optimization, it's speculative without evidence that there's actually a performance issue. The current implementation appears to compute metrics once per span during the map operation. The comment doesn't provide strong evidence that redundant computations are actually occurring, making it more of a "nice to have" suggestion rather than addressing a clear problem. This comment should be deleted. It's a speculative performance optimization suggestion without evidence of an actual problem. The current implementation appears to compute metrics once per span, and the comment doesn't demonstrate that redundant computations are actually occurring in a way that would cause performance issues.
2. frontend/components/traces/trace-view/trace-view-store-utils.ts:59
- Draft comment:
If the parent's own LLM metrics should contribute along with its children, consider merging them. Currently, when children exist, the parent's metrics are omitted. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This comment raises a valid design question about the aggregation logic. The current implementation has two branches: (1) if no children, return the span's own metrics if it's an LLM, (2) if has children, aggregate only from children. This means if a parent is itself an LLM span with its own metrics AND has children, those parent metrics are excluded. Whether this is correct depends on the intended semantics - are parent LLM spans expected to have their own metrics separate from children, or do children always represent the complete breakdown? Without seeing how this data is used or the domain requirements, I cannot definitively say this is a bug. The comment is speculative ("consider merging them") rather than definitively identifying a bug. This could be intentional design - perhaps in the trace model, if an LLM span has children, those children represent the complete breakdown of work and the parent shouldn't double-count. The comment is speculative and asks the author to "consider" something rather than pointing out a definite bug. Without more context about the domain model and requirements, I cannot be certain this is an issue. While the critique is valid that this could be intentional, the comment does identify a real behavioral difference in the code that the author may not have considered. However, the comment violates the rule about not making speculative comments - it says "consider" and "if the parent's own LLM metrics should contribute", which is speculative rather than definitive. The rules state "Do NOT make speculative comments, such as 'If X, then Y is an issue', Only comment if it is definitely an issue." This comment is speculative and asks the author to "consider" a design choice rather than identifying a definite bug. It violates the rule against speculative comments. Without strong evidence that this is definitely incorrect behavior, the comment should be deleted.
3. frontend/components/traces/trace-view/utils.ts:262
- Draft comment:
Ensure costValue is a number before calling toFixed(3) to avoid potential runtime errors. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_QaB6W2sfYB0Jb8EL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
frontend/components/traces/trace-view/trace-view-store-utils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed f22031a in 59 seconds. Click for details.
- Reviewed
33lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/trace-view-store-utils.ts:211
- Draft comment:
Good use of mapping to add aggregatedMetrics. Consider refactoring this duplicate logic (used also in transformSpansToTree) into a helper for clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. frontend/components/traces/trace-view/trace-view-store-utils.ts:244
- Draft comment:
Updating the traverse call to use childSpansWithMetrics ensures consistency. This change correctly propagates metrics to nested spans. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_Plgl2heqlaF4dHkE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed 512f3fa in 2 minutes and 3 seconds. Click for details.
- Reviewed
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/timeline-element.tsx:69
- Draft comment:
Verify that 'text-nowrap' is the correct class. In Tailwind CSS, the utility is usually 'whitespace-nowrap'. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% In Tailwind CSS, the utility for preventing text wrapping has historically beenwhitespace-nowrap. However, I recall that Tailwind CSS v3.4 introduced a newtext-nowraputility as a shorthand. So both could be valid depending on the Tailwind version. However, the comment claimstext-nowrapis incorrect and suggestswhitespace-nowrapinstead. Without knowing the exact Tailwind version being used, I cannot be 100% certain. But given thattext-nowrapwas added in Tailwind v3.4 (released in January 2024), it's likely valid if this is a modern codebase. The fact that the code would fail at build time if the class doesn't exist (it would just not apply any styles, but wouldn't break) means this isn't a critical issue. The comment is speculative about what the "correct" class is without strong evidence. I might be wrong about Tailwind v3.4 introducingtext-nowrap. Even if I'm right, the codebase might be using an older version of Tailwind where onlywhitespace-nowrapexists. The comment could be correct and helpful if the class doesn't work as intended. While I could be wrong about the Tailwind version, the key issue is that this would be caught during development/testing - iftext-nowrapdoesn't work, the text would wrap and the developer would notice. The comment is speculative ("Verify that...") which violates the rules. Without strong evidence that this is wrong, and given thattext-nowrapis a valid Tailwind utility in modern versions, I should delete this comment. The comment asks to "Verify that..." which is explicitly against the rules. Additionally,text-nowrapis a valid Tailwind CSS utility in v3.4+, and if it were invalid, it would be caught during testing. Without strong evidence that this is wrong, the comment should be deleted.
2. frontend/components/traces/trace-view/timeline-element.tsx:90
- Draft comment:
Removal of 'truncate' from commonProps may lead to text overflow. Confirm if this is the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. frontend/components/traces/trace-view/timeline-element.tsx:115
- Draft comment:
The right offset was increased from 16px to 60px. Consider adding a comment or a constant to document the reasoning behind this magic number. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_kzIgr2zQ3kZlfFdg
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed 687d9da in 2 minutes and 27 seconds. Click for details.
- Reviewed
546lines of code in8files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/timeline-element.tsx:113
- Draft comment:
The tooltip offset was adjusted from 60px to 20px (line 113). Verify that this change correctly aligns the tooltip in the timeline view. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the alignment of the tooltip after a change in offset. This falls under asking the author to double-check or ensure behavior, which is against the rules.
2. frontend/components/traces/trace-view/trace-view-store-utils.ts:141
- Draft comment:
Aggregated metrics calculation was removed from tree/timeline transformations. Ensure that consuming components correctly receive aggregated LLM metrics via the new approach. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/components/traces/trace-view/utils.ts:270
- Draft comment:
getLLMMetrics returns the cost using toFixed(3), resulting in a string. Confirm that downstream UI components expect this type or convert if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/lib/actions/spans/index.ts:307
- Draft comment:
After transforming spans with events, the new aggregateSpanMetrics function is applied. Please double-check that this aggregation produces the intended metrics compared to the previous implementation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check the aggregation function, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a specific issue.
5. frontend/lib/actions/spans/utils.ts:298
- Draft comment:
In aggregateSpanMetrics, if a span has children, its own LLM metrics are not added. Confirm that excluding the parent's own metrics when children exist is the intended aggregation behavior. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_Hg4ZVgt9tuAWVvVP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed f2f7bca in 1 minute and 13 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/components/traces/trace-view/span-card.tsx:60
- Draft comment:
Removed ref from inner div; confirm that using the ref from the outer container (line 54) is intentional and doesn't affect measurement logic. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_i8anqWuc44EzmGDW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed a67d6f6 in 1 minute and 39 seconds. Click for details.
- Reviewed
35lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/lib/actions/spans/utils.ts:299
- Draft comment:
Use consistent double quotes for the 'LLM' type check. This change from single to double quotes improves style consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. frontend/lib/actions/spans/utils.ts:316
- Draft comment:
Including the parent span’s own LLM metrics (cost and tokens) in non‐leaf spans is a good fix. Consider adding an inline comment and extra parentheses around the addition for clarity, e.g. totalCost = span.totalCost || ((span.inputCost ?? 0) + (span.outputCost ?? 0)). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. frontend/lib/actions/spans/utils.ts:345
- Draft comment:
The arrow function now wraps the parameter in parentheses. This is a minor stylistic change. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_3Psd9OT2ZrjNFJn2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| totalCost: span.totalCost, | ||
| inputTokens: span.inputTokens, | ||
| outputTokens: span.outputTokens, | ||
| totalTokens: span.totalTokens, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Pending spans incorrectly inherit child's cost/token metrics
When creating pending spans (parent spans inferred from a child's path), the cost and token metrics are incorrectly copied from the child span (span.inputCost, span.totalTokens, etc.). Pending spans represent placeholders for parents that haven't arrived yet, so they shouldn't inherit a single child's metrics. This causes incorrect metric display on pending spans and potential double-counting. The metrics for pending spans should be initialized to 0, allowing aggregateSpanMetrics to compute the correct aggregated values from actual descendants.
| const index = newSpans.findIndex((span) => span.spanId === newSpan.spanId); | ||
| if (index !== -1) { | ||
| // Always replace existing span, regardless of pending status | ||
| // Always replace existing span, regardless of pending status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Realtime updates don't compute aggregated span metrics
The onRealtimeUpdateSpans function calls enrichSpansWithPending but never calls aggregateSpanMetrics. Initial span loading via getTraceSpans computes aggregated metrics, but spans arriving via realtime updates don't get aggregatedMetrics populated. This means non-LLM parent spans won't show aggregated token/cost badges from their LLM descendants when spans arrive in real-time, and the metrics become stale as new children arrive.
| }, [span.span.name, span.events.length, span.width]); | ||
|
|
||
| const SpanText = useMemo(() => { | ||
| const spanTextElement = useMemo(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Text measurement effect has incomplete dependency array
The useLayoutEffect that measures text width for positioning depends on span.span.name, but the displayed text now uses getSpanDisplayName(span.span) which returns the model name for LLM spans. When the model name differs from the span name, the measurement won't update because the dependency array doesn't include span.span.model or span.span.spanType. This can cause incorrect text positioning when the displayed name is longer or shorter than expected.
Note
Enhances LLM span UI with model-based names, token/cost badges, and tooltips, powered by aggregated tokens/cost from descendant spans, with types/queries updated accordingly.
getSpanDisplayName()for model-based names; wrap LLM names withSpanDisplayTooltip.Coins), and cost (CircleDollarSign) inspan-card.tsxandtimeline-element.tsx.SpanStatsShieldsto acceptspanand render token/cost tooltips; show model/tools/structured output schema.aggregateSpanMetrics()to compute descendant LLM tokens/cost; apply ingetTraceSpans().getLLMMetrics()helper; update realtime updates and pending enrichment to carry token/cost fields.Span,RealtimeSpan, andTraceViewSpanwithinputTokens/outputTokens/totalTokensandinputCost/outputCost/totalCost, plusaggregatedMetricsonTraceViewSpan.StatsShieldsContentnow consumes pickedstatsfromtrace/span.Written by Cursor Bugbot for commit a67d6f6. This will update automatically on new commits. Configure here.
Important
Enhance LLM spans view with token/cost badges, tooltips, and aggregated metrics across UI and data processing components.
trace-view/span-card.tsx,trace-view/timeline-element.tsx: UsegetSpanDisplayName(); add badges for duration, tokens, and cost; display tooltip via newtrace-view/span-display-tooltip.tsx.shared/traces/span-view.tsx,traces/span-controls.tsx: SwitchSpanStatsShieldsto acceptspan; render unified stats.traces/stats-shields.tsx: Refactor toStatsShieldsContent; pass consolidatedstatsviapick; show token/cost tooltips; extract tools/structured output UI.lib/actions/spans/utils.ts: AddaggregateSpanMetrics()to compute descendant LLM tokens/cost;transformSpanWithEvents()unchanged API; wire aggregation.lib/actions/spans/index.ts: IntegrateaggregateSpanMetrics()ingetTraceSpans(); select token/cost columns.trace-view/utils.ts: AddgetSpanDisplayName()andgetLLMMetrics(); update realtime aggregation logic.trace-view/trace-view-store.tsx,lib/traces/types.ts: ExtendTraceViewSpan,Span,RealtimeSpanwith token/cost fields and optionalaggregatedMetrics.This description was created by
for a67d6f6. You can customize this summary. It will automatically update as commits are pushed.