-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(flamegraph): new trace details flamegraph #6789
feat(flamegraph): new trace details flamegraph #6789
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Reviewed everything up to fd79e16 in 1 minute and 51 seconds
More details
- Looked at
1608
lines of code in28
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/container/TraceWaterfall/TraceWaterfallStates/Success/Success.tsx:181
- Draft comment:
Theheader
function returns an empty<div />
. Ensure this is the intended behavior. - Reason this comment was not posted:
Confidence changes required:50%
Theheader
function in thegetWaterfallColumns
method returns an empty<div />
. This might be intentional, but it's worth confirming if this is the desired behavior.
2. frontend/src/hooks/trace/useGetTraceFlamegraph.tsx:15
- Draft comment:
The query keyREACT_QUERY_KEY.GET_TRACE_V2
might be incorrect for this hook. Consider using a more appropriate key likeREACT_QUERY_KEY.GET_TRACE_FLAMEGRAPH
. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The query key naming is important for react-query caching behavior. However, without seeing the REACT_QUERY_KEY enum and understanding the broader caching strategy, I can't be certain that GET_TRACE_V2 is incorrect. The current key includes both traceId and level parameters, so it will properly invalidate when needed regardless of the base key name.
I might be underestimating the importance of consistent query key naming conventions. The suggested name does seem more semantically accurate.
While better naming might be nice, the current key will function correctly and without seeing the full caching strategy, we can't be certain the suggested change is better.
Delete the comment as it's speculative without full context of the caching strategy, and the current implementation will work correctly.
3. pkg/query-service/app/clickhouseReader/reader.go:1745
- Draft comment:
The function is missing a return statement after settingtrace.TotalErrorSpansCount
. Ensure to return the trace object. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. frontend/src/AppRoutes/pageComponents.ts:47
- Draft comment:
Avoid using thecomponent/index.tsx
file structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is also applicable in other parts of the code where similar structure is used. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment references a valid architectural concern about using index.tsx files. However, this is a route configuration file that's just importing components - the actual component structure isn't being defined here. The comment is about a broader architectural pattern but isn't specifically actionable for this particular change. The change is just updating an import path to match the existing file structure.
The comment raises a valid architectural point that could improve codebase maintainability. Maybe this is exactly the right place to flag this pattern since it's where we're seeing it being used.
While the concern is valid, this routing file is not where the pattern should be addressed. The comment would be more appropriate on the PR that initially created the index.tsx structure in TraceDetailV2, not on a change that's just updating an import path to match existing structure.
Delete the comment because while it raises a valid architectural concern, it's not actionable in the context of this specific change which is just updating an import path to match existing structure.
5. frontend/src/components/TableV3/TableV3.tsx:157
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. This is also applicable in other parts of the code where inline styles are used. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The inline style in question is being used to conditionally hide/show a resize handle based on a dynamic value from getCanResize(). This is a legitimate use case for inline styles since it depends on component state. Most other inline styles in the file are also for dynamic values like sizes and transforms. These would be impractical to handle with CSS classes.
The comment raises a valid general principle about avoiding inline styles. However, it may be overlooking legitimate use cases for inline styles when dealing with dynamic values.
While the principle is good, this specific usage of inline styles is appropriate since it deals with dynamic component state that would be cumbersome to handle with CSS classes.
Delete the comment because the inline style usage here is justified for handling dynamic visibility based on component state.
Workflow ID: wflow_DqyHkifEytXM2toy
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -0,0 +1,7 @@ | |||
export enum TraceFlamegraphStates { | |||
LOADING = 'LOADING', | |||
SUCCESS = 'SUCCSS', |
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.
There are typos in the enum values. SUCCSS
should be SUCCESS
and FETCHING_WTIH_OLD_DATA_PRESENT
should be FETCHING_WITH_OLD_DATA_PRESENT
.
.span-item { | ||
position: absolute; | ||
height: 12px; | ||
background-color: yellow; |
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.
Use design tokens or predefined color constants instead of hardcoding color values like 'yellow' to maintain consistency in design and theming. This is also applicable in other parts of the code where color values are hardcoded.
Summary
flamegraph
for tracesRelated Issues / PR's
contributes to - #6132
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Add new trace details flamegraph feature with backend API and frontend integration.
SearchFlamegraphTracesV3
function inhttp_handler.go
to handle new flamegraph API endpoint.SearchFlamegraphTracesV3
inreader.go
to fetch and process flamegraph data.Reader
interface ininterface.go
to includeSearchFlamegraphTracesV3
.SearchFlamegraphTracesV3Params
andSearchFlamegraphTracesV3Response
inqueryParams.go
andresponse.go
.getTraceFlamegraph.tsx
to handle API requests for flamegraph data.TraceFlamegraph
component inPaginatedTraceFlamegraph.tsx
to display flamegraph.TraceDetailV2.tsx
to integrate flamegraph component.PaginatedTraceFlamegraph.styles.scss
andTraceDetailV2.styles.scss
.This description was created by for fd79e16. It will automatically update as commits are pushed.