feat: added logs and traces tab in host metrics detail view #6359
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c8bfe87 in 1 minute and 35 seconds
More details
- Looked at
1035lines of code in13files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:22
- Draft comment:
Theoffsetstate is initialized but never updated. If pagination or data fetching with offset is intended, ensure to update this state accordingly. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:33
- Draft comment:
Consider resettingprevOffsetandafterOffsetwhenfiltersortimeRangechanges to ensure correct data fetching. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricLogs.styles.scss:29
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values forbackgroundproperties. This applies to lines 29 and 33. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:166
- Draft comment:
Avoid using inline styles. Move the styles for theSkeletoncomponent to an external stylesheet or use styled components. This applies to lines 166 and 182. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_EURKifo6WfC4sCTK
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.
frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx
Outdated
Show resolved
Hide resolved
frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss
Outdated
Show resolved
Hide resolved
frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 698a771 in 18 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_vLunTA5PfMHU2s3E
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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> |
|
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.
👍 Looks good to me! Incremental review on 332f548 in 48 seconds
More details
- Looked at
406lines of code in10files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricsLogs/constants.ts:57
- Draft comment:
Usinguuidv4()generates a new ID for each query payload. Ensure this is intended, as it will create a unique ID for every call. - Reason this comment was not posted:
Confidence changes required:50%
The use ofuuidv4()for generating query IDs is appropriate, but it should be noted that this will generate a new ID every time the function is called, which might not be necessary if the ID is meant to be consistent across calls for the same query. If the ID is meant to be unique for each call, then this is fine.
2. frontend/src/components/HostMetricsDetail/Metrics/Metrics.tsx:37
- Draft comment:
Inconsistent time conversion: initial state divides by 1000, while custom time change divides by 1000000000. Ensure consistent units. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:159
- Draft comment:
EnsureprevQueryPayloadandafterQueryPayloadare always defined when needed, asenabled: !!prevQueryPayloadonly checks for truthiness. - Reason this comment was not posted:
Confidence changes required:50%
TheHostMetricsLogscomponent usesuseQuerywithenabled: !!prevQueryPayloadandenabled: !!afterQueryPayload. This is correct for conditional fetching, but ensure thatprevQueryPayloadandafterQueryPayloadare always defined when needed, as!!only checks for truthiness.
4. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:162
- Draft comment:
Ensureskeleton-containerstyles are defined and applied correctly. - Reason this comment was not posted:
Confidence changes required:50%
TheHostMetricsLogscomponent usesSkeletoncomponents with a class nameskeleton-container. Ensure that the styles for this class are defined and applied correctly in the corresponding SCSS file.
5. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:169
- Draft comment:
Avoid using inline styles for the Virtuoso component. Consider using CSS classes or styled components instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_4UD8tRP0ZN8IwFfh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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.
👍 Looks good to me! Incremental review on 2ecd830 in 39 seconds
More details
- Looked at
411lines of code in5files - Skipped
0files when reviewing. - Skipped posting
7drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss:15
- Draft comment:
Avoid using!importantin CSS. It can make debugging and maintenance difficult. Consider using more specific selectors or refactoring the CSS. - Reason this comment was not posted:
Confidence changes required:50%
The use of!importantin CSS should be avoided unless absolutely necessary, as it makes debugging and maintenance harder. It's better to use more specific selectors or refactor the CSS to achieve the desired effect without!important.
2. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:52
- Draft comment:
Theoffsetstate is initialized but never updated. Ensure thatoffsetis updated appropriately if pagination is required. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:54
- Draft comment:
ThesetSelectedIntervalstate is defined but never used after initialization. Consider removing it if it's not needed. - Reason this comment was not posted:
Confidence changes required:30%
ThesetSelectedIntervalstate is defined but never used after initialization. If it's not needed, consider removing it to clean up the code.
4. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:90
- Draft comment:
EnsurecurrentQuery.builder.queryDatais not empty before accessingqueryData[0]to avoid potential errors. - Reason this comment was not posted:
Confidence changes required:50%
Thequeryvariable is checked for nullability before being used in JSX. However, ifcurrentQuery.builder.queryDatais empty, accessingqueryData[0]could lead to an error. Consider adding a check forqueryDatalength before accessing its first element.
5. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:181
- Draft comment:
Ensuredata?.payload?.data?.newResult?.data?.resultis not empty before accessing its elements to avoid potential errors. - Reason this comment was not posted:
Confidence changes required:50%
ThetotalCountcalculation assumes thatdata?.payload?.data?.newResult?.data?.resultis always an array with at least one element. This could lead to errors if the structure changes or if the data is not as expected. Consider adding checks to ensure the array is not empty before accessing its elements.
6. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.styles.scss:14
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values. - Reason this comment was not posted:
Comment was on unchanged code.
7. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach. It makes it difficult to debug and find components using global search tools. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ejFFUdX13wrclcWi
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
|
List view testing - https://www.loom.com/share/daf25a3c66874aca98018de9052c1fd3 |
|
@rahulkeswani101 can we move the pagination to the bottom of the table? |
|
@GeekBoySupreme are you referring to the traces tab in detail view? We are taking the same component from traces explorer. |
|
@rahulkeswani101 traces tabs issues https://www.loom.com/share/ddb69fafe8f0468d867c0f412b971690
|
|
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.
👍 Looks good to me! Incremental review on af9f3dd in 45 seconds
More details
- Looked at
927lines of code in13files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:116
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(dateTimeRange[0] / 1000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:122
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(minTime / 1000000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricLogsDetailedView.tsx:92
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(dateTimeRange[0] / 1000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricLogsDetailedView.tsx:98
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(minTime / 1000000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/components/HostMetricsDetail/Metrics/Metrics.tsx:47
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(dateTimeRange[0] / 1000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Marked as duplicate.
6. frontend/src/components/HostMetricsDetail/Metrics/Metrics.tsx:53
- Draft comment:
Ensure consistent time conversion factors across the codebase. Here,Math.floor(minTime / 1000000000)is used, but in other places, different factors are used. Verify the correct unit and apply consistently. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_oUBKgzlv6gVLGSSQ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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.
👍 Looks good to me! Incremental review on a7710e4 in 58 seconds
More details
- Looked at
917lines of code in10files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. frontend/src/providers/QueryBuilder.tsx:919
- Draft comment:
Resetting the currentQuery to initialQueriesMap.metrics on route change to INFRASTRUCTURE_MONITORING_HOSTS may lead to unexpected behavior. Consider if this is the intended behavior. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/src/components/HostMetricsDetail/HostMetricTraces/HostMetricTraces.tsx:50
- Draft comment:
Avoid using 'any' type for the traces state. Define a specific type for better type safety and code readability. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:38
- Draft comment:
Avoid using 'any' type for the logs state. Define a specific type for better type safety and code readability. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricLogsDetailedView.tsx:37
- Draft comment:
Avoid using 'any' type for the logs state. Define a specific type for better type safety and code readability. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/src/components/HostMetricsDetail/index.tsx:54
- Draft comment:
Avoid using 'any' type for the logs state. Define a specific type for better type safety and code readability. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_9LLcxt1hzrR08O8e
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
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.
❌ Changes requested. Incremental review on aba6443 in 1 minute and 2 seconds
More details
- Looked at
745lines of code in12files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:57
- Draft comment:
TheisPaginatingstate is set but not used for any conditional logic. Consider removing it if it's not needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests thatisPaginatingis not used, but the code shows it is used in theuseQueryhook and reset in auseEffect. This means the comment is incorrect.
I might be missing some context about howisPaginatingis intended to be used, but based on the code, it is clearly used for conditional logic.
The code shows clear usage ofisPaginating, so the comment is incorrect regardless of any additional context.
The comment is incorrect becauseisPaginatingis used in the code. It should be deleted.
2. frontend/src/components/HostMetricsDetail/HostMetricsLogs/HostMetricsLogs.tsx:124
- Draft comment:
Ensure thathandleChangeLogFilterscorrectly handles pagination filters to avoid incorrect filtering. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/container/TopNav/DateTimeSelectionV2/index.tsx:390
- Draft comment:
Consider refactoringonCustomDateHandlerto improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
InDateTimeSelectionV2/index.tsx, theonCustomDateHandlerfunction has a complex logic for handling custom date selection. It might be beneficial to refactor this function to improve readability and maintainability.
4. frontend/src/pages/InfraMonitoringHosts/InfraMonitoring.styles.scss:46
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values for consistency. This is also applicable in other parts of this file. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_WJmjNZuHMtLXhLtx
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.
| handleChangeTracesFilters, | ||
| tracesFilters, | ||
| selectedInterval, | ||
| }: Props): JSX.Element { | ||
| const [traces, setTraces] = useState<any[]>([]); | ||
| const [offset] = useState<number>(0); |
There was a problem hiding this comment.
The offset state is not being updated anywhere in the component. Consider using a simple variable instead of useState.
Suggested change
| const [offset] = useState<number>(0); | |
| const offset = 0; |
| queryType: 'builder', | ||
| builder: { | ||
| ...initialQueryState.builder, | ||
| queryData: [ |
There was a problem hiding this comment.
Consider implementing duplicate filter removal in handleChangeTracesFilters similar to handleChangeLogFilters.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues / PR's
Screenshots
Affected Areas and Manually Tested Areas
Important
Add logs and traces tabs to host metrics detail view with new components, styles, and utilities.
HostMetricLogsDetailedViewandHostMetricTracescomponents for logs and traces in host metrics detail view.HostMetricDetailfor view switching.isModalTimeSelectionprop toHostDetailPropsandDateTimeSelectionV2for modal time selection.HostMetricTracesinHostMetricTraces.styles.scss.InfraMonitoring.styles.scssfor better layout and design.formatNanoToMSutility inutils.tsfor duration formatting.getHostTracesQueryPayloadandgetHostLogsQueryPayloadinconstants.tsfor query payloads.HostsList.tsxto use newHostMetricDetailwith logs and traces.DateTimeSelectionV2to support modal time selection.This description was created by
for aba6443. It will automatically update as commits are pushed.