feat(weave): adding charts to the traces page #2745
Conversation
|
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=8ba44791c37c1c0965abdd5c48beb5cda82255e3 |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
| @@ -58,7 +58,7 @@ export const SimplePageLayout: FC<{ | |||
| flexDirection: 'column', | |||
| flexGrow: 1, | |||
| height: '100%', | |||
| overflow: 'hidden', | |||
| overflow: 'hidden', // here | |||
There was a problem hiding this comment.
Leaving this comment in for now -- these are the two places that cause the issues with padding in CallsCharts. Looks like other existing margins are also not getting applied evenly
There was a problem hiding this comment.
I'm not sure I understand the //here comment. Can we just remove and we will revisit? Or maybe describe this more?
There was a problem hiding this comment.
Sorry, was just leaving for the review -- this is referencing why we need the width param at https://github.com/wandb/weave/pull/2745/files#diff-01f203878a299e8d4c183ae5ef8703a4f3cd98dd7d2ba18cf733c73ddd3a78e5R122. Will remove now!
| @@ -131,6 +131,59 @@ export const useCallsForQuery = ( | |||
| }, [callResults, calls.loading, total, costs.loading, costResults, refetch]); | |||
| }; | |||
|
|
|||
| export const useCallsForQueryCharts = ( | |||
There was a problem hiding this comment.
this does not have anything "Charts" specific, it is more of a refactor of the first section of useCallsForQuery. I'd recommend renaming this and adding a comment that it should be merged with useCallsForQuery (assuming you don't want to do that (: )
| @@ -0,0 +1,24 @@ | |||
| import {Switch} from '@wandb/weave/components'; | |||
There was a problem hiding this comment.
nit: given that this is specifically used in the calls table and not really a general component, i would co-locate it in the page/CallsPage dir
| gridPage?: GridPaginationModel, | ||
| expandedColumns?: Set<string>, | ||
| columns?: string[], | ||
| offset?: number, |
There was a problem hiding this comment.
nit: i don't prefer having parameters that override other parameters. I think you can just pass in a GridPaginationModel confirming payload. Where pageSize = limit and page = offset // limit
export interface GridPaginationModel {
/**
* Set the number of rows in one page.
* If some of the rows have children (for instance in the tree data), this number represents the amount of top level rows wanted on each page.
* @default 100
*/
pageSize: number;
/**
* The zero-based index of the current page.
* @default 0
*/
page: number;
}
| @@ -168,7 +170,8 @@ export const CallsTable: FC<{ | |||
| allowedColumnPatterns, | |||
| }) => { | |||
| const {loading: loadingUserInfo, userInfo} = useViewerInfo(); | |||
|
|
|||
| const [isMetricsChecked, setMetricsChecked] = useState(false); | |||
| console.log(isMetricsChecked); | |||
Description
See example page, https://beta.wandb.ai/vanpelt/openui-hosted/weave/calls?betaVersion=9b2425666fb0efa1f67f98cc8b55e84b00ea06d0
Adds charts for Latency, Errors, and Requests to the top of the Weave traces page. Based on figma: https://www.figma.com/design/mSF3IydmSfbUAVLCi2YlNZ/Weave-charts(hack)?node-id=65-10462&node-type=frame&t=VN4fvFeQ9QDXa3mV-0
Screen.Recording.2024-10-29.at.2.51.00.PM.mov
I would recommend moving to a different plotting library if we must support more custom tooltip styling or x axis date formatting since Plotly has limited capabilities for this.
Testing
How was this PR tested?