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: add /query-cache #288

Merged
merged 4 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/[query]/clickhouse-queries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { expensiveQueriesConfig } from './queries/expensive-queries'
import { expensiveQueriesByMemoryConfig } from './queries/expensive-queries-by-memory'
import { failedQueriesConfig } from './queries/failed-queries'
import { historyQueriesConfig } from './queries/history-queries'
import { queryCacheConfig } from './queries/query-cache'
import { runningQueriesConfig } from './queries/running-queries'
import { detachedPartsConfig } from './tables/detached-parts'
import { distributedDdlQueueConfig } from './tables/distributed-ddl-queue'
Expand All @@ -36,6 +37,7 @@ export const queries: Array<QueryConfig> = [
detachedPartsConfig,

// Queries
queryCacheConfig,
runningQueriesConfig,
historyQueriesConfig,
failedQueriesConfig,
Expand Down
42 changes: 42 additions & 0 deletions app/[query]/queries/query-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { ColumnFormat } from '@/lib/types/column-format'
import { type QueryConfig } from '@/lib/types/query-config'

export const queryCacheConfig: QueryConfig = {
name: 'query-cache',
sql: `
SELECT
query,
result_size,
formatReadableSize(result_size) AS readable_result_size,
round(100 * result_size / max(result_size) OVER ()) AS pct_result_size,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Check for division by zero.

Ensure that max(result_size) is never zero to avoid potential division by zero errors.

stale,
shared,
compressed,
expires_at,
(now() - expires_at) AS expires_in,
key_hash
FROM system.query_cache
ORDER BY expires_at DESC
LIMIT 1000
`,
columns: [
'query',
'readable_result_size',
'stale',
'shared',
'compressed',
'expires_at',
'expires_in',
'key_hash',
],
columnFormats: {
query: ColumnFormat.CodeToggle,
readable_result_size: ColumnFormat.BackgroundBar,
stale: ColumnFormat.Boolean,
shared: ColumnFormat.Boolean,
compressed: ColumnFormat.Boolean,
expires_in: ColumnFormat.Duration,
},

relatedCharts: [['query-cache', {}]],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Clarify the purpose of the empty object in relatedCharts.

If the empty object is intended for future use, consider adding a placeholder comment or a TODO to clarify its purpose.

}
43 changes: 43 additions & 0 deletions components/charts/query-cache.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { ChartCard } from '@/components/chart-card'
import { type ChartProps } from '@/components/charts/chart-props'
import { CardMetric } from '@/components/tremor/card-metric'
import { fetchData } from '@/lib/clickhouse'

export async function ChartQueryCache({
title = 'Query Cache',
className,
}: ChartProps & { name?: string }) {
const query = `
SELECT
sumIf(result_size, stale = 0) AS total_result_size,
sumIf(result_size, stale = 1) AS total_staled_result_size,
formatReadableSize(total_result_size) AS readable_total_result_size,
formatReadableSize(total_staled_result_size) AS readable_total_staled_result_size
FROM system.query_cache
`
const { data } = await fetchData<
{
total_result_size: number
total_staled_result_size: number
readable_total_result_size: string
readable_total_staled_result_size: string
}[]
>({ query })
Comment on lines +18 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Handle potential errors from fetchData.

Consider adding error handling for the fetchData call to manage potential network or query issues gracefully.

const first = data?.[0]

if (!data || !first) return null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Provide user feedback when data is not available.

Instead of returning null, consider rendering a message or a placeholder component to inform the user that no data is available.

Suggested change
if (!data || !first) return null
if (!data || !first) {
return <div>No data available</div>
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)

Suggested change
if (!data || !first) return null
if (!data || !first) {


ExplanationIt is recommended to always use braces and create explicit statement blocks.

Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).


return (
<ChartCard title={title} className={className} sql={query}>
<CardMetric
current={first.total_result_size}
currentReadable={first.readable_total_result_size + ' cached'}
duyet marked this conversation as resolved.
Show resolved Hide resolved
target={first.total_staled_result_size}
targetReadable={first.readable_total_staled_result_size + ' staled'}
className="p-2"
/>
</ChartCard>
)
}

export default ChartQueryCache
7 changes: 7 additions & 0 deletions menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { QUERY_COMMENT } from '@/lib/clickhouse'
import {
CircleDollarSignIcon,
CombineIcon,
DatabaseZapIcon,
FilePlus2Icon,
HardDriveIcon,
RollerCoasterIcon,
Expand Down Expand Up @@ -147,6 +148,12 @@ export const menuItemsConfig: MenuItem[] = [
description: 'Shows the execution plan of a statement',
icon: InfoCircledIcon,
},
{
title: 'Query Cache',
href: '/query-cache',
description: 'Query cache usage',
icon: DatabaseZapIcon,
},
Comment on lines +151 to +156
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider adding a unique identifier for the new menu item.

Adding a unique identifier (e.g., id: 'query-cache') to each menu item can help with tracking and managing menu items programmatically.

Suggested change
{
title: 'Query Cache',
href: '/query-cache',
description: 'Query cache usage',
icon: DatabaseZapIcon,
},
{
id: 'query-cache',
title: 'Query Cache',
href: '/query-cache',
description: 'Query cache usage',
icon: DatabaseZapIcon,
},

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one can respect the type when suggesting adding an id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing that out, @duyet. You're correct that the suggestion should respect the existing type definitions. If the MenuItem type does not currently include an id field, we should update the type definition accordingly. Here's an example of how you might adjust the type and the menu item:

  1. Update the MenuItem type to include an id field:

    interface MenuItem {
      id: string;
      title: string;
      href: string;
      description: string;
      icon: React.ComponentType;
    }
  2. Add the id field to the new menu item:

    {
      id: 'query-cache',
      title: 'Query Cache',
      href: '/query-cache',
      description: 'Query cache usage',
      icon: DatabaseZapIcon,
    }

Would you like me to update the type definition in the PR, or do you prefer to handle it?

],
},
{
Expand Down