Migrate to Official Clickhouse Client #2236
Conversation
|
Your preview environment pr-2236-bttf has been deployed. Preview environment endpoints are available at: |
| database: this.params.database, | ||
| application: "GrowthBook", | ||
| clickhouse_settings: { | ||
| max_execution_time: Math.min(this.params.maxExecutionTime ?? 240, 1800), |
There was a problem hiding this comment.
4 minutes feels like a really short default value. I feel like this will break people's existing data sources and require them to go in and edit the connection settings. I'd rather have the default be 30 minutes and then let them opt-in to making it shorter if they want to.
There was a problem hiding this comment.
Yeah, you're right. Not sure what I was thinking. Set it to 30 minutes, increased max to 60 minutes.
| const client = createClient({ | ||
| host: `${this.params.url}${ | ||
| this.params.port ? `:${this.params.port}` : "" | ||
| }`, |
There was a problem hiding this comment.
The old Clickhouse client supported specifying a port in the URL while also passing in a port setting. For example, this would work fine before, but would break today:
{
url: "http://localhost:1234",
port: "1234"
}There was also some weird behavior in the old client if you passed in different ports in both places, but I don't think we need to support that edge case.
There was a problem hiding this comment.
An easy solution to this is to use the URL class.
function getHost(url: string | undefined, port: number) {
if (!url) return undefined;
const host = new URL(url);
if (!host.port && port) host.port = port + "";
return host.origin;
}
// ...
const client = createClient({
host: getHost(this.params.url, this.params.port),There was a problem hiding this comment.
One issue with this is that URL(url) will throw an error if url isn't a proper URL. Should I just catch this case and do my pasting approach or should we consider some better validation when the form is saved (I don't think we do any right now).
There was a problem hiding this comment.
for example, do something like this to catch some edge cases
export function getHost(url: string | undefined, port: number): string | undefined {
if (!url) return undefined;
try {
const host = new URL(url);
if (!host.port && port) host.port = port + "";
return host.origin;
} catch (e) {
return `${url}:${port}`;
}
}
There was a problem hiding this comment.
I think it's fine to just let it throw. I'm pretty sure if they have an invalid URL today it will be broken, so this shouldn't introduce any regressions.
|
new stats look good default now showing in form |
Features and Changes
Move to official clickhouse client.
Closes #2194
May resolve #2130 error which seems to be emitted from the clickhouse client we were using before.
Testing
Ran test, experiment metric and fact metric queries, and traffic query for ClickHouse. All works as expected.
Screenshots
Statistics show up:
Timeout setting:
Timeout works:
