ref(insights): Refactor Insights -> LLM charts #90094

billyvg · 2025-04-22T19:52:10Z

Refactors the Insights -> LLM charts to better support deep linking in a future PR.

Notable in this PR is that these widgets are used in 3 different sections:

Landing Page - no additional args
Details Page - takes a groupId from URL parameters
"LLM Monitor Section" (group details page) - The wrapping components takes in an event argument, which uses the trace (span id + trace id) to query for a groupId to pass to widgets

We're going to refactor the 3 widgets into:

~~(3) base widgets that accept an optional groupId (referring to AI Pipeline Group, and not our normal Issues Group)~~
(3) widgets that do not require a groupId from URL params. These widgets handle point 1) above
(3) widgets that use groupId from URL parameters. These widgets handle points ~~1) and~~ 2) above.
(2) widgets (llmEvent*) that take groupId (Issues Group!) and eventId from URL parameters to fetch the Issues Group, and then tries to find the AI Pipeline Group id via the event's trace. This AI Pipeline Group ID is then used in the query and renders the appropriate charts. These widgets account for point 3) above.

In order to handle this¹ (and for future widgets), this PR introduces a chartProperties prop for LoadableChartWidgetProps. These are to be used as URL query params when linking to the Releases Drawer, but also used to render the chart as well. They are chart-specific, which is the reason for the additional level of object nesting. Think of it as these two stages:

~~- "Serialization": we have a chart, and want to "serialize" it into a URL. This will involve 2 steps~~
- taking the keys of chartProperties object and using a known query parameter key (e.g. rdChartPropertyKey) to create a list of query parameters (e.g. rdChartPropertyKey=foo&rdChartPropertyKey=bar)
~~- turning chartProperties into query parameters (e.g. foo=val1&bar=val2)~~

~~- "Parsing": inverse of "serialization" -- we have a URL and want to render a chart.~~
~~- using known key (rdChartPropertyKey), we're able to fetch the keys of chartProperties (which can vary from chart to chart)~~
~~- with the keys, we can again look through the query parameters to get the values for each key~~

~~The known key (e.g. rdChartPropertKey) acts as an indirect pointer to the chartProperties object which has dynamic keys. See this commit for an example.~~

Part of #88560

There is another solution, since (1) and (2) can be handled without any changes, we would just need slightly diff widgets for (3) where it's mostly the same except it expects eventId to available via URL params and it fetches the event to get the trace and then fetches the group id ↩

Refactors the Insights -> LLM charts to better support deep linking in a future PR. Notable in this PR is the introduction of a `chartProperties` prop for `LoadableChartWidgetProps`. These are to be used as URL query params when linking to the Releases Drawer, but also used to render the chart as well. They are chart-specific, which is the reason for the additional level of object nesting. Part of #88560

codecov · 2025-04-22T19:56:59Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
10278	1	10277	5

View the top 1 failed test(s) by shortest run time

EventSearch handles basic inputs for tags

Stack Traces | 5.07s run time

Error: thrown: "Exceeded timeout of 5000 ms for a test.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."
    at .../issueDetails/streamline/eventSearch.spec.tsx:51:3
    at _dispatchDescribe (.../jest-circus/build/index.js:91:26)
    at describe (.../jest-circus/build/index.js:55:5)
    at Object.<anonymous> (.../issueDetails/streamline/eventSearch.spec.tsx:15:1)
    at Runtime._execModule (.../jest-runtime/build/index.js:1439:24)
    at Runtime._loadModule (.../jest-runtime/build/index.js:1022:12)
    at Runtime.requireModule (.../jest-runtime/build/index.js:882:12)
    at jestAdapter (.../build/legacy-code-todo-rewrite/jestAdapter.js:77:13)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
    at runTestInternal (.../jest-runner/build/runTest.js:367:16)
    at runTest (.../jest-runner/build/runTest.js:444:34)
    at Object.worker (.../jest-runner/build/testWorker.js:106:12)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

billyvg · 2025-04-22T21:39:53Z

@gggritso this is slightly more complex than the other widgets. I have a slightly more complicated solution here that can account for future widgets where we need extra URL parameters for a specific chart. But the footnote above describes a potentially simpler approach that requires more queries.

gggritso · 2025-04-23T16:17:39Z

I spent all morning thinking about this, going crazy Charlie-and-the-corkboard style, and couldn't come up with anything better. I'm a little spooked by the rdChartPropertyKey approach.

I'm confused about the widget loader. It pulls the parameters off the URL and passes them to the loaded component. When someone clicks a release bubble in a main chart, and the URL is updated to open the panel chart, won't that reload the main chart, and re-filter it?
Passing this many bubble specific parameters through the visualization component doesn't feel great. Maybe that concern is out of scope for this PR?
Managing the URL this way might be hard, I'm wondering if it'll be annoying to have to clear the parameters when closing the panel, since the panel might not be aware of which parameters were used to open the chart.
We veered away from having the chart query configuration in the URL, and this feels like a step in that direction, which also doesn't feel great.

My main question is about this:

a slightly more complicated solution here that can account for future widgets where we need extra URL parameters for a specific chart

Are the specific examples of this? This LLM chart, as far as I can tell, is the only one that's rendered in an Issues context. Are there others? Do they have similar limitations?

If this is the only one, I think having three widgets (one for LLM landing page, one for LLM pipeline details page, and one for the Issue Details page) is way simpler, and matches how the other widgets are done (e.g., query duration). The extra request is annoying, but that seems like a price worth paying.

billyvg · 2025-04-23T22:30:36Z

I'm confused about the widget loader. It pulls the parameters off the URL and passes them to the loaded component. When someone clicks a release bubble in a main chart, and the URL is updated to open the panel chart, won't that reload the main chart, and re-filter it?

Passing this many bubble specific parameters through the visualization component doesn't feel great. Maybe that concern is out of scope for this PR?

Managing the URL this way might be hard, I'm wondering if it'll be annoying to have to clear the parameters when closing the panel, since the panel might not be aware of which parameters were used to open the chart.

We veered away from having the chart query configuration in the URL, and this feels like a step in that direction, which also doesn't feel great.

All great points, 1+3 are addressable but comes w/ complexity costs. 2+4 are good reasons to avoid this solution.

My main question is about this:

a slightly more complicated solution here that can account for future widgets where we need extra URL parameters for a specific chart

Are the specific examples of this? This LLM chart, as far as I can tell, is the only one that's rendered in an Issues context. Are there others? Do they have similar limitations?

Nope, no others thus far (well aside from EventGraph which we can handle without chartProperties too).

You're right, there's no reason to move away from our initial design regarding chart configuration in the URL. I've gone ahead and removed chartProperties -- the charts should follow the same design and patterns of our other charts!

billyvg · 2025-04-23T22:11:54Z

static/app/components/events/interfaces/llm-monitoring/llmMonitoringSection.tsx

-  const {data} = useSpansIndexed(
-    {
-      limit: 1,
-      fields: [SpanIndexedField.SPAN_AI_PIPELINE_GROUP],
-      search: new MutableSearch(`trace:${trace?.trace_id} id:"${trace?.span_id}"`),
-      enabled: Boolean(trace?.span_id) && Boolean(trace?.trace_id),
-    },
-    'api.ai-pipelines.view'
-  );
-
-  const aiPipelineGroup = data[0]?.[SpanIndexedField.SPAN_AI_PIPELINE_GROUP];


Moved into a new hook that is used in the new widgets

billyvg · 2025-04-23T22:12:04Z

static/app/components/events/interfaces/llm-monitoring/llmMonitoringSection.tsx

-          </ModuleLayout.Half>
-        </ModuleLayout.Layout>
-      ) : (
-        'loading'


billyvg · 2025-04-23T22:16:08Z

static/app/components/events/interfaces/llm-monitoring/llmMonitoringSection.tsx

This is definitely a yolo refactor, I don't have an issue that renders this component. @gggritso helped me look at some Sentry traces and it doesn't seem like this is used at all (at least in last 30 days)

gggritso

I'd be just as happy (maybe even happier) with 9 widgets and no base, TBH. I think it's a little sketchy that the useParams() intentionally doesn't return a groupId on pages without that param, which is half the time, due to the specific page it's on.

Not major though, this is pretty straight-forward overall 👍🏻

billyvg · 2025-04-24T18:44:25Z

👍 got rid of base* charts and made discrete components.

github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 22, 2025

vercel bot deployed to Preview April 22, 2025 20:00 View deployment

pass pageFilters

9cc14a4

vercel bot deployed to Preview April 22, 2025 21:32 View deployment

billyvg requested a review from gggritso April 22, 2025 21:37

refactor to remove chartProperties

60771e8

vercel bot deployed to Preview April 23, 2025 22:15 View deployment

revert changes

4a10f67

vercel bot deployed to Preview April 23, 2025 22:22 View deployment

billyvg commented Apr 23, 2025

View reviewed changes

billyvg marked this pull request as ready for review April 23, 2025 22:30

billyvg requested review from a team as code owners April 23, 2025 22:30

billyvg requested a review from a team April 23, 2025 22:31

gggritso approved these changes Apr 24, 2025

View reviewed changes

remove base* widgets

2ffb132

billyvg requested a review from a team as a code owner April 24, 2025 17:56

vercel bot deployed to Preview April 24, 2025 18:00 View deployment

imports

fbb0c20

vercel bot deployed to Preview April 24, 2025 18:05 View deployment

imports

d9cce33

vercel bot deployed to Preview April 24, 2025 18:11 View deployment

copy and paste bugs

ee7f0b9

vercel bot deployed to Preview April 24, 2025 18:41 View deployment

billyvg enabled auto-merge (squash) April 24, 2025 18:44

billyvg merged commit e7fc468 into master Apr 24, 2025
45 checks passed

billyvg deleted the billy/replay-76-refactor-charts-in-llmmonitoringcharts branch April 24, 2025 18:52

github-actions bot locked and limited conversation to collaborators May 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

ref(insights): Refactor Insights -> LLM charts #90094

ref(insights): Refactor Insights -> LLM charts #90094

Uh oh!

billyvg commented Apr 22, 2025 •

edited

Loading

Uh oh!

codecov bot commented Apr 22, 2025 •

edited

Loading

Uh oh!

billyvg commented Apr 22, 2025

Uh oh!

gggritso commented Apr 23, 2025

Uh oh!

billyvg commented Apr 23, 2025

Uh oh!

billyvg Apr 23, 2025

Uh oh!

billyvg Apr 23, 2025

Uh oh!

billyvg Apr 23, 2025

Uh oh!

gggritso left a comment

Uh oh!

billyvg commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ref(insights): Refactor Insights -> LLM charts #90094

ref(insights): Refactor Insights -> LLM charts #90094

Uh oh!

Conversation

billyvg commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

codecov bot commented Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ 1 Tests Failed:

Uh oh!

billyvg commented Apr 22, 2025

Uh oh!

gggritso commented Apr 23, 2025

Uh oh!

billyvg commented Apr 23, 2025

Uh oh!

billyvg Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

billyvg Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

billyvg Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

gggritso left a comment

Choose a reason for hiding this comment

Uh oh!

billyvg commented Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

billyvg commented Apr 22, 2025 •

edited

Loading

codecov bot commented Apr 22, 2025 •

edited

Loading