Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: share filters across logs and traces views in entity details #7014

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

amlannandy
Copy link
Member

@amlannandy amlannandy commented Feb 3, 2025

Summary

Share filters across logs and traces views in entity details so that filers applied in one view carry over to the other

Related Issues / PR's

Fixes #6877

Screenshots

NA

Affected Areas and Manually Tested Areas

Infra Monitoring


Important

Unified filter state for logs and traces across Kubernetes entity detail views, ensuring consistent filter application.

  • Behavior:
    • Unified filter state logsAndTracesFilters for logs and traces in ClusterDetails.tsx, DaemonSetDetails.tsx, DeploymentDetails.tsx, and 8 other files.
    • Updated handleChangeLogFilters and handleChangeTracesFilters to use logsAndTracesFilters.
    • Introduced filterOutPrimaryFilters in utils.tsx to manage filter items.
  • Files:
    • Modified ClusterDetails.tsx, DaemonSetDetails.tsx, DeploymentDetails.tsx to use unified filter state.
    • Updated EntityLogsDetailedView.tsx and EntityTraces.tsx to apply filterOutPrimaryFilters.
  • Misc:
    • Removed redundant filter states and logic for individual logs and traces filters.

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

@amlannandy amlannandy requested a review from YounixM as a code owner February 3, 2025 12:58
@CLAassistant
Copy link

CLAassistant commented Feb 3, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the chore label Feb 3, 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.

👍 Looks good to me! Reviewed everything up to 6656db4 in 1 minute and 44 seconds

More details
  • Looked at 859 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Clusters/ClusterDetails/ClusterDetails.tsx:141
  • Draft comment:
    Good refactoring – merging logFilters and tracesFilters into a single state (logsAndTracesFilters) simplifies management of shared filters. Ensure the filtered primary filters (using QUERY_KEYS.K8S_CLUSTER_NAME) correctly persist in both views.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/src/container/InfraMonitoringK8s/DaemonSets/DaemonSetDetails/DaemonSetDetails.tsx:155
  • Draft comment:
    The consolidation of log and traces filters into logAndTracesFilters is clear. Verify that any view‐specific exclusions (e.g., filtering out pagination or resource keys) still behave as expected.
  • Reason this comment was not posted:
    Marked as duplicate.
3. frontend/src/container/InfraMonitoringK8s/Deployments/DeploymentDetails/DeploymentDetails.tsx:157
  • Draft comment:
    Refactoring looks consistent—using a unified ‘logAndTracesFilters’ state improves shared filter handling across logs and traces. Double-check that queryKeyFilters are passed when needed for the TRACES view.
  • Reason this comment was not posted:
    Marked as duplicate.
4. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityLogs/EntityLogsDetailedView.tsx:62
  • Draft comment:
    Using the utility function filterOutPrimaryFilters in the memo dependencies is a good improvement. Ensure that dependency arrays include all necessary values so updated filters trigger a recalculation.
  • Reason this comment was not posted:
    Comment did not seem useful.
5. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityTraces/EntityTraces.tsx:79
  • Draft comment:
    The unified filter state usage in constructing the updated current query for traces is good. Confirm that the dependency array for useMemo properly tracks changes to queryKeyFilters and tracesFilters.items.
  • Reason this comment was not posted:
    Marked as duplicate.
6. frontend/src/container/InfraMonitoringK8s/Jobs/JobDetails/JobDetails.tsx:171
  • Draft comment:
    The changes to merge logFilters and tracesFilters into a single state are consistent. The updated filtering callbacks appear correct. Make sure that the primary filters (using QUERY_KEYS.K8S_JOB_NAME) are consistently maintained.
  • Reason this comment was not posted:
    Marked as duplicate.
7. frontend/src/container/InfraMonitoringK8s/Namespaces/NamespaceDetails/NamespaceDetails.tsx:161
  • Draft comment:
    Shared filter state (logAndTracesFilters) is applied across logs and traces views. Verify that the QUERY_KEYS for namespaces (K8S_NAMESPACE_NAME, K8S_CLUSTER_NAME) are preserved in the filtering logic.
  • Reason this comment was not posted:
    Marked as duplicate.
8. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/NodeDetails.tsx:161
  • Draft comment:
    The refactoring to use logAndTracesFilters is clean. Ensure that both primary filters (based on K8S_NODE_NAME and K8S_CLUSTER_NAME) and pagination filters work as expected in the logs view.
  • Reason this comment was not posted:
    Marked as duplicate.
9. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodDetails.tsx:178
  • Draft comment:
    The consolidation into logsAndTracesFilters is consistent here. Confirm that all queries (logs, traces, events) correctly use the shared filter state and that dependency arrays in useEffect properly reset when initialFilters change.
  • Reason this comment was not posted:
    Marked as duplicate.
10. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:177
  • Draft comment:
    The merging of logFilters and tracesFilters into logAndTracesFilters and proper passing to metrics/logs/traces views looks good. Double-check that the filtering callbacks preserve the essential keys (e.g., QUERY_KEYS.K8S_STATEFUL_SET_NAME, K8S_NAMESPACE_NAME).
  • Reason this comment was not posted:
    Marked as duplicate.
11. General:1
  • Draft comment:
    Overall, the refactoring to share filter state between logs and traces tabs effectively resolves issue Carry resource filter selection b/w logs/traces tabs in the detailed view #6877. One suggestion: avoid using inline styles in React components (e.g., Drawer style objects); use external CSS or styled components for consistency and 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.
12. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:88
  • Draft comment:
    Good refactoring using a combined filters state. Consider standardizing the variable name across components (e.g. consistently use either ‘logsAndTracesFilters’ or ‘logAndTracesFilters’).
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
13. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityLogs/EntityLogsDetailedView.tsx:61
  • Draft comment:
    Ensure that the dependency array for the useMemo (line 69) correctly tracks changes to queryKeyFilters and logFilters.items. Consider memoizing the queryKeyFilters array if it’s defined inline.
  • Reason this comment was not posted:
    Marked as duplicate.
14. Multiple files:390
  • Draft comment:
    Consider moving inline style objects (e.g. Drawer style for background and overscrollBehavior) to external CSS classes or styled components to adhere to best practices.
  • 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.
15. Multiple files:205
  • Draft comment:
    The useCallback hooks throughout the files disable react-hooks/exhaustive-deps. Verify that omitting dependencies is intentional to avoid potential stale closure issues.
  • 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.
16. All modified details view components:140
  • Draft comment:
    The refactoring to share filter state across logs and traces views appears consistent and meets the issue requirements (Carry resource filter selection b/w logs/traces tabs in the detailed view #6877). Ensure that end-to-end testing confirms that a filter applied in one view automatically carries over to the other.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None

Workflow ID: wflow_xg5lHT0JgAxWZCBm


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@amlannandy amlannandy force-pushed the chore/share-filters-logs-traces branch from 6656db4 to 2b7e7be Compare February 3, 2025 13:04
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 2b7e7be in 1 minute and 26 seconds

More details
  • Looked at 859 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Clusters/ClusterDetails/ClusterDetails.tsx:144
  • Draft comment:
    Good refactor: separate log and traces filters are unified into one state (logsAndTracesFilters) to share filtering between views. Make sure the primary filters (by K8S_CLUSTER_NAME) are preserved correctly when merging new and pagination filters.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
2. frontend/src/container/InfraMonitoringK8s/DaemonSets/DaemonSetDetails/DaemonSetDetails.tsx:154
  • Draft comment:
    The new unified state 'logAndTracesFilters' is used consistently for logs and traces views. Verify that primary keys for daemon set filters ([K8S_DAEMON_SET_NAME, K8S_NAMESPACE_NAME]) are correctly kept in merging operations.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
3. frontend/src/container/InfraMonitoringK8s/Deployments/DeploymentDetails/DeploymentDetails.tsx:177
  • Draft comment:
    The shared 'logAndTracesFilters' state is smoothly integrated in DeploymentDetails. Confirm that the queryKeyFilters prop is passed when needed.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
4. frontend/src/container/InfraMonitoringK8s/Jobs/JobDetails/JobDetails.tsx:170
  • Draft comment:
    The merging of job filters into 'logAndTracesFilters' appears consistent. Please ensure that the filter keys for jobs ([K8S_JOB_NAME, K8S_NAMESPACE_NAME]) are used consistently in both log and trace filters functions.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
5. frontend/src/container/InfraMonitoringK8s/Namespaces/NamespaceDetails/NamespaceDetails.tsx:211
  • Draft comment:
    NamespaceDetails uses the unified filters correctly. Check that queryKeyFilters passed to NamespaceLogs/Traces are as intended.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
6. frontend/src/container/InfraMonitoringK8s/Nodes/NodeDetails/NodeDetails.tsx:212
  • Draft comment:
    In NodeDetails, the shared logsAndTracesFilters state is integrated properly for logs and traces views. Ensure that the dependency arrays in useEffect callbacks are adequate, even with ESLint disables.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
7. frontend/src/container/InfraMonitoringK8s/Pods/PodDetails/PodDetails.tsx:226
  • Draft comment:
    PodDetails now uses unified logsAndTracesFilters state that carries across logs and traces. Confirm that TimeRangeOffset usage is consistent and that filtering works using keys [K8S_POD_NAME, K8S_NAMESPACE_NAME].
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
8. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:246
  • Draft comment:
    StatefulSetDetails consistently uses unified filters for both logs and traces. Verify that the QUERY_KEYS for stateful sets ([K8S_STATEFUL_SET_NAME, K8S_NAMESPACE_NAME]) are consistently applied.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
9. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityLogs/EntityLogsDetailedView.tsx:60
  • Draft comment:
    EntityLogsDetailedView correctly uses filterOutPrimaryFilters to remove primary keys based on queryKeyFilters. Verify that dependency array includes logFilters.items and queryKeyFilters.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
10. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityTraces/EntityTraces.tsx:76
  • Draft comment:
    EntityTraces now removes primary filters from tracesFilters via filterOutPrimaryFilters. Check that its dependency array is comprehensive.
  • Reason this comment was not posted:
    Confidence changes required: 0%
    None
11. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:340
  • Draft comment:
    Overall, the unified state helps share the filters across tabs correctly. Consider moving inline style definitions (e.g., Drawer style) to external CSS or styled components for consistency with best practices.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
12. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:161
  • Draft comment:
    Naming inconsistency: In some files the combined filter state is named 'logAndTracesFilters' and in others 'logsAndTracesFilters'. Consider standardizing this naming across components for consistency.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  1. The comment claims there's an inconsistency but doesn't provide clear evidence of both naming variants.
  2. The second code snippet doesn't even contain either variable name.
  3. Without seeing clear evidence of the inconsistency, we can't verify if this is a real issue.
  4. Even if true, variable naming consistency across files is more of a nice-to-have than a critical issue.
    I might be missing other files in the codebase where this inconsistency exists. The reviewer may have access to more context than what's shown here.
    While there could be inconsistencies elsewhere, we need clear evidence to justify keeping a comment. Without it, we risk creating unnecessary work based on unverified claims.
    Delete this comment as there isn't sufficient evidence provided to verify the claimed naming inconsistency across files.
13. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:394
  • Draft comment:
    Inline styles are used on the Drawer component. Consider moving these styles to external stylesheets or styled components to adhere to best practices.
  • 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.
14. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:221
  • Draft comment:
    Multiple hooks disable exhaustive-deps via eslint comments. Ensure that the dependency arrays for useEffect and useCallback are complete, as missing dependencies can lead to stale closures.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
15. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:225
  • Draft comment:
    The logic in handleChangeLogFilters and handleChangeTracesFilters is nearly identical. Consider refactoring the common filtering logic into a shared utility to reduce duplication.
  • Reason this comment was not posted:
    Comment was on unchanged code.
16. Multiple Files:0

Workflow ID: wflow_P6K3AS5uFAf3TGpu


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

SagarRajput-7
SagarRajput-7 previously approved these changes Feb 5, 2025
@amlannandy amlannandy force-pushed the chore/share-filters-logs-traces branch from 2b7e7be to a323111 Compare February 5, 2025 10:24
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 a323111 in 41 seconds

More details
  • Looked at 859 lines of code in 11 files
  • Skipped 0 files when reviewing.
  • Skipped posting 11 drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/Clusters/ClusterDetails/ClusterDetails.tsx:141
  • Draft comment:
    Merging log and traces filters into a single state (logsAndTracesFilters) makes the code cleaner. Ensure that all dependent callbacks correctly update and use the merged filters.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
2. frontend/src/container/InfraMonitoringK8s/DaemonSets/DaemonSetDetails/DaemonSetDetails.tsx:155
  • Draft comment:
    The consolidation of log and traces filters into logAndTracesFilters was applied here. Verify that filtering logic (primary filters extraction and exclusion of pagination filters) remains consistent across both logs and traces handlers.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
3. frontend/src/container/InfraMonitoringK8s/Deployments/DeploymentDetails/DeploymentDetails.tsx:157
  • Draft comment:
    Ensure that merging filters into logAndTracesFilters consistently reflects in both the logs and traces explorers. The queryKeyFilters prop is correctly added for the traces view.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
4. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/EntityLogs/EntityLogsDetailedView.tsx:61
  • Draft comment:
    The dependency array in the useMemo for updating the current query now includes logFilters.items and queryKeyFilters. This should ensure that changes to the merged filter state trigger query updates.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
5. frontend/src/container/InfraMonitoringK8s/StatefulSets/StatefulSetDetails/StatefulSetDetails.tsx:252
  • Draft comment:
    Review the handleChangeLogFilters callback to ensure that the filtering excludes the proper keys after merging. It appears to correctly exclude 'id' and the primary key for stateful set name.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
6. Multiple Files:400
  • Draft comment:
    Minor: Inline styles for Drawer components use design tokens (Color, Spacing). Ensure consistency in styling across components; consider using CSS classes or styled components if the project prefers to avoid inline styles for easier theming and debugging.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None
7. ClusterDetails.tsx:144
  • Draft comment:
    Inconsistent naming: the shared filter state is called 'logsAndTracesFilters' in some components and 'logAndTracesFilters' in others. Standardize the naming 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.
8. EntityLogsDetailedView.tsx:60
  • Draft comment:
    Good use of a helper (filterOutPrimaryFilters) to remove redundant primary filters. Ensure this abstraction is maintained consistently in all log and trace components.
  • Reason this comment was not posted:
    Confidence changes required: 20%
    None
9. Various Components (e.g., ClusterDetails.tsx, PodDetails.tsx, StatefulSetDetails.tsx):390
  • Draft comment:
    Several components use inline styles (e.g. Drawer 'style' prop). Although design tokens are used, consider moving these styles to a CSS module or styled component for better maintainability.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None
10. Multiple Files (e.g., ClusterDetails.tsx, DaemonSetDetails.tsx, DeploymentDetails.tsx, JobDetails.tsx):214
  • Draft comment:
    Many callbacks (e.g., handleTimeChange, handleChangeLogFilters) disable ESLint dependency warnings. Verify that all variables used are stable, or include proper dependencies to avoid potential stale closures.
  • 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.
11. Multiple Components:240
  • Draft comment:
    The logic merging primary filters with new filters in handleChangeLogFilters and handleChangeTracesFilters is duplicated across several components. Consider abstracting this filter-merge logic into a shared utility or custom hook to reduce duplication and ensure consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    None

Workflow ID: wflow_OjFsaZDxRKST2luZ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

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. Incremental review on 32c653b in 47 seconds

More details
  • Looked at 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/EntityDetailsUtils/utils.tsx:312
  • Draft comment:
    Ensure excluding filters with key 'id' is intentional. Please add a comment noting why 'id' filters are omitted from sharing across views.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    None

Workflow ID: wflow_59a8jcgSjDH5EsPq


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.

@amlannandy amlannandy merged commit f548afe into main Feb 5, 2025
17 checks passed
@amlannandy amlannandy deleted the chore/share-filters-logs-traces branch February 5, 2025 12:48
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carry resource filter selection b/w logs/traces tabs in the detailed view
3 participants