feat: add CLICKHOUSE_MAX_EXECUTION_TIME, update fetchData
#264
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Hey @duyet - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 11 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 4 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| <TopRightToolbarExtras database={database} table={table} /> | ||
| } | ||
| config={config} | ||
| data={columns} |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with data structure
Ensure that columns is structured correctly for the DataTable component. The change from fetchDataWithCache() to destructuring might have altered the expected data format.
| query: sql, | ||
| format: 'JSONEachRow', | ||
| clickhouse_settings: { use_query_cache: 1, query_cache_ttl: 120 }, | ||
| }) | ||
|
|
||
| if (!data || !data.length || !data?.[0]?.['count()']) return null |
There was a problem hiding this comment.
suggestion: Redundant null check
The null check !data is redundant since data is destructured from the response and will always be defined. Consider removing it.
| if (!data || !data.length || !data?.[0]?.['count()']) return null | |
| if (!data.length || !data?.[0]?.['count()']) return null |
| }: { | ||
| web?: B | ||
| clickhouse_settings?: ClickHouseSettings |
There was a problem hiding this comment.
suggestion: Missing documentation for new parameter
Consider adding a comment or documentation for the new clickhouse_settings parameter to explain its purpose and usage.
| clickhouse_settings?: ClickHouseSettings | |
| // Optional settings for configuring the ClickHouse client | |
| clickhouse_settings?: ClickHouseSettings |
| @@ -10,14 +10,16 @@ import { seeding } from './seeding' | |||
| export const getCustomDashboards = async () => { | |||
| await seeding() | |||
|
|
|||
| const dashboards = await fetchData<TableChartsRow[]>({ | |||
| const q1 = fetchData<TableChartsRow[]>({ | |||
There was a problem hiding this comment.
suggestion (performance): Potential performance improvement
Consider using Promise.all directly without intermediate variables q1 and q2 to potentially improve readability and performance.
| const q1 = fetchData<TableChartsRow[]>({ | |
| await Promise.all([ | |
| fetchData<TableChartsRow[]>({ | |
| query: `SELECT * FROM ${TABLE_CHARTS} FINAL ORDER BY ordering ASC`, | |
| }) | |
| ]); |
| databases = await fetchDataWithCache()({ | ||
| query: listDatabases, | ||
| }) | ||
| const { data: databases }: { data: DatabaseCount[] } = |
There was a problem hiding this comment.
suggestion: Type annotation redundancy
The type annotation { data: DatabaseCount[] } is redundant since TypeScript can infer the type from the function return type. Consider removing it for cleaner code.
| const { data: databases }: { data: DatabaseCount[] } = | |
| const { data: databases } = |
| try { | ||
| data = await fetchData<{ 'count()': string }[]>({ | ||
| const { data } = await fetchData<{ 'count()': string }[]>({ |
There was a problem hiding this comment.
issue (complexity): Consider reverting to a structure similar to the original while incorporating the necessary changes.
The new code introduces more complexity compared to the previous version. Here are some points to consider:
-
Inline Changes: The new code has inline changes within the
tryblock, making it harder to read and understand. The previous version separated data fetching and data validation logic, which was easier to follow. -
Error Handling: While the error handling is now at the end of the function, the inline changes make it less clear where potential points of failure are.
-
Readability: The previous version had a clear separation of concerns—fetching data, checking data, and rendering the component. The new version mixes these steps together, reducing readability.
-
Maintainability: The previous version was more modular, making it easier to maintain and update individual parts of the function. The new version's inline approach makes it harder to isolate and update specific logic.
Consider reverting to a structure similar to the original while incorporating the necessary changes. Here's a revised version:
export async function CountBadge({
sql,
className,
variant = 'outline',
}: CountBadgeProps): Promise<JSX.Element | null> {
if (!sql) return null;
let data: { 'count()': string }[] = [];
try {
const response = await fetchData<{ 'count()': string }[]>({
query: sql,
format: 'JSONEachRow',
clickhouse_settings: { use_query_cache: 1, query_cache_ttl: 120 },
});
data = response.data;
} catch (e: any) {
console.error(
`<CountBadge />: could not get count for sql: ${sql}, error: ${e}`
);
return null;
}
if (!data || !data.length || !data?.[0]?.['count()']) return null;
const count = data[0]['count()'] || 0;
if (count == 0) return null;
return (
<Badge className={className} variant={variant}>
{count}
</Badge>
);
}This version maintains the original structure, making it easier to read and maintain while still incorporating the necessary changes.
| @@ -18,16 +20,24 @@ export const getClickHouseHost = () => getClickHouseHosts()[0] | |||
|
|
|||
| export const getClient = <B extends boolean>({ | |||
| web, | |||
| clickhouse_settings, | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the getClient function and handling of clickhouse_settings.
The new code introduces additional complexity in several ways:
-
Increased Parameter Complexity: The
getClientfunction now takes an additionalclickhouse_settingsparameter, complicating the function signature and requiring the caller to provide this parameter. -
Nested Object Spread: The nested object spread within the
clickhouse_settingsobject can be error-prone and harder to read, especially for those unfamiliar with the spread operator or the specific structure ofclickhouse_settings. -
Default Value Handling: Introducing a default value for
max_execution_timeusing a constantDEFAULT_CLICKHOUSE_MAX_EXECUTION_TIMEadds another layer of complexity, as the default value is now managed in two places: the environment variable and the constant. -
Return Type Change: Changing the return type of the
fetchDatafunction fromPromise<T>toPromise<{ data: T }>affects all places wherefetchDatais called, requiring updates to handle the new structure of the returned data.
To address these issues while maintaining readability, consider the following simplified approach:
import type { ClickHouseClient, DataFormat } from '@clickhouse/client';
import { createClient } from '@clickhouse/client';
import type { ClickHouseSettings, QueryParams } from '@clickhouse/client-common';
import { createClient as createClientWeb } from '@clickhouse/client-web';
import type { WebClickHouseClient } from '@clickhouse/client-web/dist/client';
import { cache } from 'react';
const DEFAULT_CLICKHOUSE_MAX_EXECUTION_TIME = 60;
export const getClickHouseHosts = () => {
const hosts = (process.env.CLICKHOUSE_HOST || '')
.split(',')
.map((host) => host.trim())
.filter(Boolean);
return hosts;
};
export const getClickHouseHost = () => getClickHouseHosts()[0];
export const getClient = <B extends boolean>({
web,
clickhouse_settings = {},
}: {
web?: B;
clickhouse_settings?: ClickHouseSettings;
}): B extends true ? WebClickHouseClient : ClickHouseClient => {
const client = web === true ? createClientWeb : createClient;
return client({
host: getClickHouseHost(),
username: process.env.CLICKHOUSE_USER ?? 'default',
password: process.env.CLICKHOUSE_PASSWORD ?? '',
clickhouse_settings: {
max_execution_time: parseInt(
process.env.CLICKHOUSE_MAX_EXECUTION_TIME ?? DEFAULT_CLICKHOUSE_MAX_EXECUTION_TIME.toString()
),
...clickhouse_settings,
},
}) as B extends true ? WebClickHouseClient : ClickHouseClient;
};
export const QUERY_COMMENT = '/* { "client": "clickhouse-monitoring" } */ ';
export const fetchData = async <T>({
query,
query_params,
format = 'JSONEachRow',
clickhouse_settings,
}: QueryParams): Promise<T> => {
const start = new Date();
const client = getClient({ web: false, clickhouse_settings });
const resultSet = await client.query({
query: QUERY_COMMENT + query,
format,
query_params,
clickhouse_settings,
});
const data = await resultSet.json<T>();
const query_id = resultSet.query_id;
if (data && 'statistics' in data) {
console.debug(
`<-- Response (${query_id}):`,
'rows',
JSON.stringify(data.statistics),
'\n'
);
} else if (typeof data === 'object' && data.hasOwnProperty('rows')) {
console.debug(`<-- Response (${query_id}): ${data.rows} rows\n`);
} else {
console.debug(`<-- Response (${query_id}):`, data, '\n`);
}
return data;
};
export const fetchDataWithCache = () => cache(fetchData);
export const query = async (
query: string,
params: Record<string, unknown> = {},
format: DataFormat = 'JSON'
) => {
const resultSet = await getClient({ web: false }).query({
query,
format,
query_params: params,
});
return resultSet.json();
};This approach maintains the new functionality while keeping the code more readable and easier to maintain.
| const first = rows?.[0] | ||
| if (!rows || !first) return null | ||
| const first = data?.[0] | ||
| if (!data || !first) return null |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!data || !first) return null | |
| if (!data || !first) { |
Explanation
It 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).
| query: sql, | ||
| format: 'JSONEachRow', | ||
| clickhouse_settings: { use_query_cache: 1, query_cache_ttl: 120 }, | ||
| }) | ||
|
|
||
| if (!data || !data.length || !data?.[0]?.['count()']) return null |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (!data || !data.length || !data?.[0]?.['count()']) return null | |
| if (!data || !data.length || !data?.[0]?.['count()']) { |
Explanation
It 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).
| if (!data || !data.length || !data?.[0]?.['count()']) return null | ||
|
|
||
| const count = data[0]['count()'] || 0 | ||
| if (count == 0) return null |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (count == 0) return null | |
| if (count == 0) { |
Explanation
It 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).
No description provided.