Skip to content
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

feat(llm-observability): spans #27933

Merged
merged 25 commits into from
Jan 28, 2025
Merged

feat(llm-observability): spans #27933

merged 25 commits into from
Jan 28, 2025

Conversation

skoob13
Copy link
Contributor

@skoob13 skoob13 commented Jan 27, 2025

Problem

Users want to see a tree of spans and generations with the input and output of each step.

Changes

  • Restore the generation tree.
  • Render the tree recursively.
  • Update taxonomy for spans.
  • Update the traces query runner to have a fallback for $ai_trace_name and return $ai_span events.

Light theme

Screenshot 2025-01-28 at 11 22 20

Dark theme

Screenshot 2025-01-28 at 11 22 08

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

Unit tests and manual testing

Copy link
Contributor

github-actions bot commented Jan 27, 2025

Size Change: +503 B (+0.04%)

Total Size: 1.16 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.16 MB +503 B (+0.04%)

compressed-size-action

@skoob13 skoob13 marked this pull request as ready for review January 28, 2025 10:31
@skoob13 skoob13 requested review from Twixes and k11kirky January 28, 2025 10:31
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tree overflow gets janky, otherwise all good

</h3>
</div>
{isLLMTraceEvent(event) ? (
<MetadataHeader
hasError={event.properties.$ai_is_error}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming nit: may be more consistent to do isError={event.properties.$ai_is_error}

)
}
)
LLMMessageDisplay.displayName = 'LLMMessageDisplay'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't be needed if the memo used function:

export const LLMMessageDisplay = React.memo(
    function LLMMessageDisplay(...) { }
)

topLevelTrace,
item,
isSelected,
const TraceNode = React.memo(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually this is a TraceLeaf

)
TraceNode.displayName = 'TraceNode'

function RecursiveTreeDisplay({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is a TraceNode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AH, I was being raving mad, this is actually NodeChildren and the other one is TreeNode

</span>
),
]
const hasChildren = children.find((child) => !!child)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const hasChildren = children.find((child) => !!child)
const hasChildren = children.some((child) => !!child)

Copy link
Contributor Author

@skoob13 skoob13 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

find is enough here. We just need to know that at least one child exists. You're right😄 It's what some does. I'm more used to find.

const idMap = new Map<any, LLMTraceEvent>()

// Map all events with parents to their parent IDs
events.forEach((event) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always find for (... of ...) a bit more readable, but that's a nit which should be a linter rule

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forEach used to be faster. I'm unsure though it's true now.

}): JSX.Element {
return (
<aside className="border-border max-h-fit bg-bg-light border rounded overflow-hidden md:w-72">
<aside className="border-border max-h-full bg-bg-light border rounded overflow-hidden md:w-72">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional we're back to filling the full height of the scene even if there's just a couple items in the trace? I believed max-h-fit would still work here

Copy link
Contributor Author

@skoob13 skoob13 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, if we set max-h-fit, the scroll won't work for larger content.

</NestingGroup>
</aside>
)
}

function NestingGroup({ level = 0, children }: { level?: number; children: React.ReactNode }): JSX.Element {
const listEl = <ul className={!level ? 'overflow-y-auto p-1 first:*:mt-0' : 'flex-1'}>{children}</ul>
const listEl = (
<ul className={!level ? 'hide-scrollbar overflow-y-auto p-1 first:*:mt-0 h-full' : 'flex-1'}>{children}</ul>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few snags in scrolling:

  1. It's a much worse experience without scrollbars, let's avoid hide-scrollbar
  2. Horizontal overflow is a little awkward, it's generally hard to make scrolling a list in both axes feel good. Proposing we wrap for readability instead. This will also avoid mismatches width, like here:
    Screenshot 2025-01-28 at 16 31 08
  3. I believe this should be max-h-full so avoid filling height with whitespace

Copy link
Contributor Author

@skoob13 skoob13 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we usually show scrollbars for overflow or not? The scrollbar doesn't look nice on Windows.

Copy link
Contributor Author

@skoob13 skoob13 Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's all fixed, but the scrollbar question is nice to know.

Screenshot 2025-01-28 at 19 22 00

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do! On macOS it's worth setting "always show scrollbars" in system settings, to see them (though most macOS users won't)
Screenshot 2025-01-28 at 21 27 16

const usage = formatLLMUsage(item)
return (
<NestingGroup level={1}>
{tree.map(({ event, children }) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something mysterious I'm seeing in Max AI traces: a span beyond what I can scroll to at the bottom
Screenshot 2025-01-28 at 16 33 21

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I didn't notice it.

Comment on lines 306 to 307
'p-2 text-xs border rounded',
!raisedError ? 'bg-[var(--bg-fill-success-tertiary)]' : 'bg-[var(--bg-fill-warning-tertiary)]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use red to signal an error:

Suggested change
'p-2 text-xs border rounded',
!raisedError ? 'bg-[var(--bg-fill-success-tertiary)]' : 'bg-[var(--bg-fill-warning-tertiary)]'
'p-2 text-xs border rounded',
!raisedError ? 'bg-[var(--bg-fill-success-tertiary)]' : 'bg-[var(--bg-fill-danger-tertiary)]'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the danger color initially, but the contrast ratio is not great and is hard to read. When you look at those spans for a little longer, you actually want it to be plain and readable. I've thought about changing the header color instead of applying a background, as it serves the same indicative purpose, but the content is significantly easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error

Screenshot 2025-01-28 at 18 20 44 Screenshot 2025-01-28 at 18 21 00

Danger

Screenshot 2025-01-28 at 18 21 44 Screenshot 2025-01-28 at 18 21 53

I'll switch to red, as I think it looks better now. Let's see.

@skoob13 skoob13 requested a review from Twixes January 28, 2025 18:22
Copy link
Member

@Twixes Twixes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@k11kirky k11kirky merged commit d7758ee into master Jan 28, 2025
99 checks passed
@k11kirky k11kirky deleted the feat/llm-observability-spans branch January 28, 2025 21:45
adamleithp pushed a commit that referenced this pull request Jan 29, 2025
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Michael Matloka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants