Skip to content
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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

amlannandy
Copy link
Member

@amlannandy amlannandy commented Mar 7, 2025

Summary

Implement metric details view in a side drawer for metrics explorer

  • Search among attributes
  • Edit metadata

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.

  • Behavior:
    • Adds MetricDetails component in MetricDetails.tsx to display metric details in a side drawer.
    • Implements getMetricDetails in getMetricDetails.ts to fetch metric details from the API.
    • Implements updateMetricMetadata in updateMetricMetadata.ts to update metric metadata.
    • Allows searching and filtering of attributes in AllAttributes.tsx.
    • Displays associated dashboards and alerts in DashboardsAndAlertsPopover.tsx.
    • Allows editing of metric metadata in Metadata.tsx.
  • Styles:
    • Adds styles for metric details components in MetricDetails.styles.scss.
  • Hooks:
    • Adds useGetMetricDetails in useGetMetricDetails.ts for fetching metric details.
    • Adds useUpdateMetricMetadata in useUpdateMetricMetadata.ts for updating metric metadata.
  • Misc:
    • Updates Summary.tsx to integrate the metric details view.
    • Adds GET_METRIC_DETAILS to reactQueryKeys.ts.
    • Updates MetricsTable.tsx and MetricsTreemap.tsx to open metric details on interaction.

This description was created by Ellipsis for 4846a38. It will automatically update as commits are pushed.

@amlannandy amlannandy requested a review from YounixM as a code owner March 7, 2025 11:35
@github-actions github-actions bot added enhancement New feature or request docs required labels Mar 7, 2025
@amlannandy amlannandy self-assigned this Mar 7, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 21 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% <= threshold 50%
    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.

@@ -0,0 +1,67 @@
import { Button, Collapse, Typography } from 'antd';
Copy link
Member Author

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

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 3 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs not required enhancement New feature or request
Projects
None yet
1 participant