-
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: implement metric details view in metrics explorer #7238
base: main
Are you sure you want to change the base?
Conversation
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 77b9512 in 3 minutes and 22 seconds
More details
- Looked at
1610
lines of code in21
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. frontend/src/hooks/metricsExplorer/useUpdateMetricMetadata.ts:23
- Draft comment:
The useMutation setup is clear and follows best practices. No issues detected. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. frontend/src/container/MetricsExplorer/Summary/utils.tsx:112
- Draft comment:
Avoid using inline styles in MetricTypeRenderer; move the dynamic styling (backgroundColor, border, color) to a separate CSS class that leverages design tokens. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. frontend/src/container/MetricsExplorer/Summary/utils.tsx:121
- Draft comment:
Avoid inline styling on Typography.Text; externalize style definitions to maintain consistency across the design. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/api/metricsExplorer/getMetricDetails.ts:13
- Draft comment:
The property 'timeseries' on line 13 is in a different case style compared to 'timeSeriesTotal' and 'timeSeriesActive'. Consider renaming it to 'timeSeries' for consistency, unless this naming is intentional due to API contracts. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Since this is an interface definition that likely maps to an API response, the property names probably need to match the backend API contract. The comment itself acknowledges this by saying "unless this naming is intentional due to API contracts". Without seeing the API contract, we can't be sure this is actually an issue that needs fixing. The comment is speculative.
The inconsistency in naming could make the code less readable and maintainable if it's not API-driven. If it is a legitimate issue, fixing it would improve code quality.
However, the comment itself acknowledges uncertainty about API contracts, and we don't have visibility into the backend API. Making this change could break the API integration.
Delete the comment because it's speculative and we don't have enough context to be certain that this is an issue that needs fixing. The property name might be intentionally matching an API contract.
5. frontend/src/api/metricsExplorer/getMetricDetails.ts:20
- Draft comment:
The property 'metric_type' inside the 'metadata' object (line 20) uses snake_case, while most of the interface properties use camelCase. Ensure this inconsistency is intentional (e.g., matching API response format); otherwise, consider aligning it with camelCase for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment asks to "ensure this is intentional" which violates our rule about not asking for confirmations. The snake_case appears to be intentional as it's consistently used for API response fields throughout the file. This looks like a typical pattern of maintaining API contract naming while using camelCase for internal fields.
Maybe there's a broader inconsistency issue that needs to be addressed across the codebase? Maybe there should be a type transformation layer?
While those are interesting points, they're outside the scope of this PR review. We should focus on clear issues with the changed code, not speculative improvements.
Delete the comment as it violates the rule about asking for confirmations and appears to be questioning an intentional pattern seen throughout the file.
6. frontend/src/constants/reactQueryKeys.ts:18
- Draft comment:
Typo: 'GET_ALL_ALLERTS' appears to be misspelled. Consider correcting it to 'GET_ALL_ALERTS'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/constants/reactQueryKeys.ts:33
- Draft comment:
The string value for 'GET_DAEMONSET_LIST' includes an extraneous comma (i.e., 'GET_DAEMONSET_LIST,'). Please remove the comma so that it reads 'GET_DAEMONSET_LIST'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. frontend/src/container/MetricsExplorer/Summary/MetricsTreemap.tsx:38
- Draft comment:
Typo: 'extracedTreemapData' should be corrected to 'extractedTreemapData'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_jA48UR3xXgkjvjER
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/container/MetricsExplorer/MetricDetails/DashboardsAndAlertsPopover.tsx
Show resolved
Hide resolved
frontend/src/container/MetricsExplorer/MetricDetails/DashboardsAndAlertsPopover.tsx
Show resolved
Hide resolved
frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/src/container/MetricsExplorer/MetricDetails/Metadata.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,67 @@ | |||
import { Button, Collapse, Typography } from 'antd'; |
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.
@sawhil This is the component you need to use
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.
👍 Looks good to me! Incremental review on 4846a38 in 2 minutes and 21 seconds
More details
- Looked at
70
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. frontend/src/container/MetricsExplorer/MetricDetails/Metadata.tsx:108
- Draft comment:
Good removal of debug logging. Ensure no essential log is omitted. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that no essential log is omitted after removing debug logging. This falls under the category of asking the author to double-check or ensure something, which is against the rules provided. Therefore, this comment should be removed.
2. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.styles.scss:28
- Draft comment:
Consider using design tokens for spacing values instead of hardcoded '12px' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
While using design tokens is generally good practice and matches the additional rules provided, I notice several issues: 1) We don't see evidence that var(--spacing-medium) actually exists in the codebase 2) The file uses many raw pixel values for spacing throughout 3) The comment doesn't provide strong evidence that this specific change is required 4) The suggestion seems more like a general best practice than a specific issue.
I might be too harsh - using design tokens is explicitly mentioned in the additional rules. Also, the presence of other CSS variables shows the infrastructure exists.
While design tokens are good, we need stronger evidence that var(--spacing-medium) exists and is the correct token to use here. Making this change without that evidence could introduce errors.
Delete the comment. While the suggestion aligns with best practices, we lack evidence that the suggested token exists and is appropriate here.
3. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:88
- Draft comment:
Avoid inline styles on the Drawer; consider moving these styles to an external stylesheet or CSS class for better maintainability. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/container/MetricsExplorer/MetricDetails/Metadata.tsx:108
- Draft comment:
Good removal of the debugging console.log. Ensure no leftover debug logs remain in production. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that there are no debug logs left in production. This falls under the category of asking the author to double-check something, which is against the rules provided.
5. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.styles.scss:28
- Draft comment:
Gap reduced from 32px to 12px; please double-check that this spacing change aligns with the overall design specifications. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:88
- Draft comment:
Avoid inline styles in React components. Consider refactoring the Drawer style to a CSS class or styled component for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:94
- Draft comment:
The inline margin on the close icon was removed. Verify that the close icon's vertical alignment remains correct after this change. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:127
- Draft comment:
Extraneous div wrapper around TopAttributes, Metadata, and AllAttributes has been removed, simplifying the markup. Confirm that the layout still meets design requirements. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
9. frontend/src/container/MetricsExplorer/MetricDetails/MetricDetails.tsx:52
- Draft comment:
Minor naming inconsistency: Consider renaming 'goToMetricsExplorerwithSelectedMetric' to 'goToMetricsExplorerWithSelectedMetric' (capitalizing the 'W') for improved readability and consistency with camelCase naming conventions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_DjMkFm3bmhpqYPfA
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Implement metric details view in a side drawer for metrics explorer
Related Issues / PR's
closes #7040
closes #7044
closes #7045
Screenshots
NA
Affected Areas and Manually Tested Areas
Summary tab in Metrics Explorer
Important
Implement metric details view in metrics explorer with search, edit, and associated dashboards/alerts features.
MetricDetails
component inMetricDetails.tsx
to display metric details in a side drawer.getMetricDetails
ingetMetricDetails.ts
to fetch metric details from the API.updateMetricMetadata
inupdateMetricMetadata.ts
to update metric metadata.AllAttributes.tsx
.DashboardsAndAlertsPopover.tsx
.Metadata.tsx
.MetricDetails.styles.scss
.useGetMetricDetails
inuseGetMetricDetails.ts
for fetching metric details.useUpdateMetricMetadata
inuseUpdateMetricMetadata.ts
for updating metric metadata.Summary.tsx
to integrate the metric details view.GET_METRIC_DETAILS
toreactQueryKeys.ts
.MetricsTable.tsx
andMetricsTreemap.tsx
to open metric details on interaction.This description was created by
for 4846a38. It will automatically update as commits are pushed.