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 http-connections and interserver-connections charts #242

Merged
merged 3 commits into from
May 18, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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/api/timezone/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { NextResponse } from 'next/server'

import { fetchData } from '@/lib/clickhouse'

export const revalidate = false
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Clarify the purpose of disabling revalidation.

It would be helpful to add a comment explaining why revalidation is disabled for this route. This can provide context for future maintainers.

Suggested change
export const revalidate = false
export const revalidate = false // Revalidation is disabled to ensure the data remains static and doesn't trigger unnecessary fetches


export async function GET() {
try {
const resp = await fetchData<{ tz: string }[]>({
Expand Down
29 changes: 0 additions & 29 deletions app/charts/[chart]/page.tsx

This file was deleted.

44 changes: 44 additions & 0 deletions app/charts/[charts]/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { ChartSkeleton } from '@/components/skeleton'
import { notFound } from 'next/navigation'
import { Suspense } from 'react'

interface PageProps {
params: {
charts: string
}
}

export const dynamic = 'force-dynamic'
export const revalidate = 30

export default async function Page({ params: { charts } }: PageProps) {
let chartComponents = []
let props = {}

for (const chart of decodeURIComponent(charts).split(',')) {
console.log(`Rendering chart: ${chart}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Remove or replace console.log statements.

Consider removing or replacing console.log statements with a proper logging mechanism to avoid cluttering the console in production.

Suggested change
console.log(`Rendering chart: ${chart}`)
import { log } from 'some-logging-library';
...
log.info(`Rendering chart: ${chart}`);

try {
chartComponents.push(
(await import(`@/components/charts/${chart}`)).default
)
} catch (e) {
console.error(`Error rendering chart: ${chart}`, e)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Improve error handling for chart rendering.

Consider providing more user-friendly feedback or fallback UI in case of errors when rendering charts, rather than just logging the error.

Suggested change
console.error(`Error rendering chart: ${chart}`, e)
console.error(`Error rendering chart: ${chart}`, e)
alert('An error occurred while rendering the chart. Please try again later.')

notFound()
}
}

return (
<div>
{chartComponents.map((Chart, i) => (
<Suspense key={i} fallback={<ChartSkeleton />}>
<Chart
key={i}
className="mb-4 w-full p-0 shadow-none"
chartClassName="h-64"
{...props}
/>
</Suspense>
))}
</div>
)
}
50 changes: 50 additions & 0 deletions components/charts/connections-http.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { ChartCard } from '@/components/chart-card'
import { fetchData } from '@/lib/clickhouse'
import { cn } from '@/lib/utils'
import { BarChart } from '../tremor/bar'
import { type ChartProps } from './chart-props'

export async function ChartConnectionsHttp({
title = 'HTTP Connections Last 7 days (Total Requests / Hour)',
interval = 'toStartOfHour',
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 issue (security): Consider validating the interval parameter.

To prevent potential SQL injection or unexpected behavior, consider validating the interval parameter to ensure it only contains allowed values.

lastHours = 24 * 7,
className,
chartClassName,
}: ChartProps) {
const query = `
SELECT
${interval}(event_time) AS event_time,
SUM(CurrentMetric_HTTPConnection) AS CurrentMetric_HTTPConnection,
formatReadableQuantity(CurrentMetric_HTTPConnection) AS readable_CurrentMetric_HTTPConnection
FROM system.metric_log
WHERE event_time >= now() - INTERVAL ${lastHours} HOUR
GROUP BY event_time
ORDER BY event_time
`

const data = await fetchData<
{
event_time: string
CurrentMetric_HTTPConnection: number
readable_CurrentMetric_HTTPConnection: string
}[]
>({
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): Handle potential errors from fetchData.

Consider adding error handling for the fetchData call to manage potential failures gracefully.

query,
format: 'JSONEachRow',
})

return (
<ChartCard title={title} sql={query} className={className}>
<BarChart
data={data}
index="event_time"
categories={['CurrentMetric_HTTPConnection']}
readableColumn="readable_CurrentMetric_HTTPConnection"
className={cn('h-52', chartClassName)}
stack
/>
</ChartCard>
)
}

export default ChartConnectionsHttp
50 changes: 50 additions & 0 deletions components/charts/connections-interserver.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { ChartCard } from '@/components/chart-card'
import { fetchData } from '@/lib/clickhouse'
import { cn } from '@/lib/utils'
import { BarChart } from '../tremor/bar'
import { type ChartProps } from './chart-props'

export async function ChartConnectionsInterserver({
title = 'Interserver Connections Last 7 days (Total Requests / Hour)',
interval = 'toStartOfHour',
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 issue (security): Consider validating the interval parameter.

To prevent potential SQL injection or unexpected behavior, consider validating the interval parameter to ensure it only contains allowed values.

lastHours = 24 * 7,
className,
chartClassName,
}: ChartProps) {
const query = `
SELECT
${interval}(event_time) AS event_time,
SUM(CurrentMetric_InterserverConnection) AS CurrentMetric_InterserverConnection,
formatReadableQuantity(CurrentMetric_InterserverConnection) AS readable_CurrentMetric_InterserverConnection
FROM system.metric_log
WHERE event_time >= now() - INTERVAL ${lastHours} HOUR
GROUP BY event_time
ORDER BY event_time
`

const data = await fetchData<
{
event_time: string
CurrentMetric_InterserverConnection: number
readable_CurrentMetric_InterserverConnection: string
}[]
>({
Comment on lines +25 to +31
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): Handle potential errors from fetchData.

Consider adding error handling for the fetchData call to manage potential failures gracefully.

query,
format: 'JSONEachRow',
})

return (
<ChartCard title={title} sql={query} className={className}>
<BarChart
data={data}
index="event_time"
categories={['CurrentMetric_InterserverConnection']}
readableColumn="readable_CurrentMetric_InterserverConnection"
className={cn('h-52', chartClassName)}
stack
/>
</ChartCard>
)
}

export default ChartConnectionsInterserver
7 changes: 7 additions & 0 deletions menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
HardDriveIcon,
RollerCoasterIcon,
UngroupIcon,
UnplugIcon,
} from 'lucide-react'
import { type MenuItem } from './components/menu/types'

Expand Down Expand Up @@ -247,6 +248,12 @@ export const menuItemsConfig: MenuItem[] = [
'Exposes data from the Keeper cluster defined in the config',
icon: RollerCoasterIcon,
},
{
title: 'Connections',
href: '/charts/connections-http,connections-interserver',
description: 'Number of connections over time',
icon: UnplugIcon,
},
Comment on lines +251 to +256
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 each menu item.

Adding a unique identifier (e.g., id) to each menu item can help with tracking and managing menu items more effectively, especially if the list grows or needs to be manipulated programmatically.

Suggested change
{
title: 'Connections',
href: '/charts/connections-http,connections-interserver',
description: 'Number of connections over time',
icon: UnplugIcon,
},
{
id: 'connections',
title: 'Connections',
href: '/charts/connections-http,connections-interserver',
description: 'Number of connections over time',
icon: UnplugIcon,
},

],
},
]