feat: add charts to failed-query page #367
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Reviewer's Guide by SourceryThis pull request adds charts to the failed-query page and enhances the existing query configuration. The changes include updating the SQL query, modifying column formats, and introducing new chart components for visualizing failed query data. File-Level Changes
Sequence DiagramsequenceDiagram
participant User
participant FailedQueryPage
participant ClickHouse
participant ChartFailedQueryCount
participant ChartFailedQueryCountByType
User->>FailedQueryPage: View failed queries
FailedQueryPage->>ClickHouse: Fetch failed query data
ClickHouse-->>FailedQueryPage: Return query results
FailedQueryPage->>ChartFailedQueryCount: Render failed query count chart
ChartFailedQueryCount->>ClickHouse: Fetch chart data
ClickHouse-->>ChartFailedQueryCount: Return chart data
FailedQueryPage->>ChartFailedQueryCountByType: Render failed query count by user chart
ChartFailedQueryCountByType->>ClickHouse: Fetch chart data
ClickHouse-->>ChartFailedQueryCountByType: Return chart data
FailedQueryPage-->>User: Display failed queries and charts
Tips
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes - here's some feedback:
Overall Comments:
- The change in the CodeDialogFormat component test from 'button svg[role="open-dialog"]' to 'svg[role="open-dialog"]' seems significant. Can you confirm this is intentional and doesn't affect other parts of the system?
- Consider adding documentation for the new chart components (ChartFailedQueryCount and ChartFailedQueryCountByType) to facilitate reuse and maintenance.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| FROM system.query_log | ||
| WHERE type IN ['ExceptionBeforeStart', 'ExceptionWhileProcessing'] | ||
| ORDER BY query_start_time DESC | ||
| LIMIT 1000 |
There was a problem hiding this comment.
suggestion (performance): Consider implementing pagination instead of increasing the LIMIT
Increasing the LIMIT from 100 to 1000 could potentially impact query performance. Consider implementing a pagination mechanism to fetch results in smaller batches while still allowing access to a larger dataset.
LIMIT 100
OFFSET :offset
| initial_user, | ||
| multiIf(empty(client_name), http_user_agent, concat(client_name, ' ', toString(client_version_major), '.', toString(client_version_minor), '.', toString(client_version_patch))) AS client, | ||
| client_hostname, | ||
| toString(databases) AS databases, |
There was a problem hiding this comment.
suggestion: Consider providing a way to view full, structured data
While using toString() makes the data more easily displayable in a table format, it might lose information for complex structures. Consider adding a way to view the full, structured data on demand, perhaps through a expandable cell or a detailed view option.
| toString(databases) AS databases, | |
| toString(databases) AS databases, | |
| JSON.stringify(databases) AS databases_full, |
| import { fetchData } from '@/lib/clickhouse' | ||
| import { applyInterval } from '@/lib/clickhouse-query' | ||
|
|
||
| export async function ChartFailedQueryCountByType({ |
There was a problem hiding this comment.
issue: Correct inconsistent naming between file and component
The file is named 'failed-query-count-by-user.tsx', but the component is named 'ChartFailedQueryCountByType'. This inconsistency could lead to confusion. Consider renaming the component to match the file name or vice versa.
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #367 +/- ##
=======================================
Coverage ? 75.00%
=======================================
Files ? 6
Lines ? 152
Branches ? 55
=======================================
Hits ? 114
Misses ? 38
Partials ? 0 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
Add charts to the failed-query page to visualize failed queries over time and by user. Enhance the failed queries configuration with additional descriptions and increased query limits. Update column formats for better data presentation and adjust tests for the CodeDialogFormat component.
New Features:
Enhancements:
Tests: