feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755

knudtty · 2025-04-16T14:32:08Z

Ref: HDX-1381

changeset-bot · 2025-04-16T14:32:12Z

🦋 Changeset detected

Latest commit: 454f7b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/common-utils	Patch
@hyperdx/api	Patch
@hyperdx/app	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

knudtty · 2025-04-16T14:33:04Z

packages/api/src/routers/api/clickhouseProxy.ts

-  validateRequest({
-    query: z.object({
-      hyperdx_connection_id: objectIdSchema,
+function validation() {


handling both GET and POST routes now, so broke into RequestHandlers that both can use

style: we don't need to wrap the validate middleware within another function, right? can just be a var

Yeah, I was doing it mainly to make the PR easily reviewable (other functions would reformat and indent lines that otherwise haven't changed) but I'll just add some prettier commands for the time being

knudtty · 2025-04-16T14:37:25Z

packages/api/src/routers/api/clickhouseProxy.ts

    try {
      const { teamId } = getNonNullUserWithTeam(req);
-      const { hyperdx_connection_id } = req.query;
+      const connection_id = req.headers['x-hyperdx-connection-id']!; // ! because zod already validated


Header x-hyperdx-connection-id is now in place of the respective queryparam. I chose this based on the clickhouse documentation for a reverse-proxy in front of a clickhouse instance. They do have a section that allows you to send query params as well, but those are all prefixed with 'param_', which quickly gets messy with zod validating either query param 'hyperdx_connection_id' or 'param_hyperdx_connection_id', so I opted to move to using headers.

knudtty · 2025-04-16T14:38:35Z

packages/api/src/routers/api/clickhouseProxy.ts

+        const qIdx = _req.url.indexOf('?');
+        if (qIdx >= 0) {
+          qparams = _req.url.substring(qIdx);
+        }


_req.query is type ParamQs, which does not necessarily play nicely with URLSearchParams. Since we now forward all query params, prefer to just grab the string from the url

will this introduce any encoding issue? I'd suggest to move all irrelevant changes to a separate PR so folks can review it easier

knudtty · 2025-04-16T14:39:22Z

packages/api/src/utils/validation.ts

+import express from 'express';
+import { z } from 'zod';
+
+export function validateRequestHeaders<T extends z.Schema>(schema: T) {


Basically the same as validateRequest function, but they don't validate headers. Maybe should submit a feature request

knudtty · 2025-04-16T14:49:24Z

packages/app/pages/api/[...all].ts

-     **/
-    on: {
-      proxyReq: fixRequestBody,
-    },
    // ...(IS_DEV && {


The function fixRequestBody does not handle Content-Type: 'text/plain', which makes things horribly annoying. Here's what happens:

bodyParser.text reads the incoming stream to req.body

fixRequestBody looks at the content type and sees 'text/plain', but it doesn't handle that, so it does not write to the proxy request

The proxy request is sent to our express server with a non-zero content-length but an empty body

The express server's express.text middleware (bodyParser under the hood) will see text/plain and a Content-Length > 0, so it tries to read the incoming stream and parse it into req.body

The stream never comes in, so next() is never called. The server will just wait ad infinitum.

I filed an issue and PR to fix fixRequestBody. But we don't even need bodyParser here, so just disabling it and forwarding everything works too.

packages/app/src/sessions.ts

packages/common-utils/src/clickhouse.ts

knudtty · 2025-04-16T14:52:19Z

packages/common-utils/src/clickhouse.ts

+      const headers = {};
+      if (!isLocalMode && connectionId) {
+        headers['x-hyperdx-connection-id'] = connectionId;
+      }


change to sending connectionId through the header instead of query params

packages/app/src/sessions.ts

MikeShi42 · 2025-04-16T16:21:42Z

packages/app/src/sessions.ts

      );

+      const clickhouseClient = createClient({


why not reuse or extend?

hyperdx/packages/common-utils/src/clickhouse.ts

Line 260 in 3f14f88

export class ClickhouseClient {

I talked with Warren, this would be a good proof of concept first to use client-web here and then introduce to ClickhouseClient when we feel good about it. I created a linear issue to do that

can you also add a TODO comment?

MikeShi42 · 2025-04-16T16:23:30Z

packages/app/src/sessions.ts

+
+// OPTIMIZATION STRATEGY
+//
+// 1. Write a clickhouse query to divide a session into different chunks, where each chunk has a start time. Maybe each chunk contains 100 events.


chunks can only start from a full snapshot (type 4 rrweb event), as most events are deltas that need to be applied from the last full snapshot.

MikeShi42 · 2025-04-16T16:28:04Z

packages/app/src/sessions.ts

+// 1. Write a clickhouse query to divide a session into different chunks, where each chunk has a start time. Maybe each chunk contains 100 events.
+// 2. When slider advances, use the timestamp to determine which chunk you are in
+// 3. Fetch data associated with that chunk
+// 4. Probably do some prefetching for future times
 export function useRRWebEventStream(


I'm wondering why we don't reuse useOffsetPaginatedQuery here instead? iirc it should do all the things this function does, but better. we should avoid multiple streaming clickhouse query function implementations to keep our implementation centralized

Good question. I do think we need to refactor how rrweb events are fetched in general. I created HDX-1629 for that

vercel · 2025-04-17T19:47:05Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hyperdx-v2-oss-app	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 22, 2025 9:39pm

wrn14897 · 2025-04-17T23:04:55Z

packages/api/src/utils/validation.ts

@@ -0,0 +1,17 @@
+import express from 'express';


nit: we should probably put this in the middleware dir

wrn14897 · 2025-04-17T23:10:51Z

packages/app/src/sessions.ts

+          'x-hyperdx-connection-id': source.connection,
+        },
+        url: window.location.origin,
+        pathname: '/api/clickhouse-proxy',


since the app is able to stream directly, the clickhouse client should request ch server without proxy under local model. check out the getClickhouseClient method (

hyperdx/packages/app/src/clickhouse.ts

Lines 21 to 39 in 8c95b9e

export const getClickhouseClient = () => {

if (IS_LOCAL_MODE) {

const localConnections = getLocalConnections();

if (localConnections.length === 0) {

console.warn('No local connection found');

return new ClickhouseClient({

host: '',

});

}

return new ClickhouseClient({

host: localConnections[0].host,

username: localConnections[0].username,

password: localConnections[0].password,

});

}

return new ClickhouseClient({

host: PROXY_CLICKHOUSE_HOST,

});

};

).

Great, added in

wrn14897 · 2025-04-17T23:21:57Z

packages/app/pages/api/[...all].ts

@@ -6,7 +6,8 @@ const DEFAULT_SERVER_URL = `http://127.0.0.1:${process.env.HYPERDX_API_PORT}`;
 export const config = {
  api: {
    externalResolver: true,
-    bodyParser: true,
+    bodyParser: false,
+    responseLimit: '32mb',


Can you elaborate on the decision behind this number?

I saw it as good practice to implement a responseLimit online and felt it might not be best security to allow unlimited data to flow through, so I chose a number I also saw being used on the express bodyParser middleware for json and text in api-app.ts. This may not be ideal though, especially since we already allow the client to potentially send a massive query to the db, which is where the performance hit would really come from.

For now I'm removing the responseLimit. I'll look to your guidance for the right thing to do on this

packages/app/src/sessions.ts

wrn14897 · 2025-04-21T17:09:11Z

packages/app/src/sessions.ts

+            }
+          : undefined,
+        url: hostname ?? window.location.origin,
+        pathname: pathname ?? '/api/clickhouse-proxy',


nit: can reuse PROXY_CLICKHOUSE_HOST

That is local to the clickhouse client file in common-utils. I was thinking when we move this over to common-utils we would then swap this to PROXY_CLICKHOUSE_HOST. I'll add a todo comment for that

wrn14897 · 2025-04-21T17:16:52Z

packages/app/src/sessions.ts

+              'x-hyperdx-connection-id': source.connection,
+            }
+          : undefined,
+        url: hostname ?? window.location.origin,


I got this error while testing the demo sessions. I suspect its due to the validation from the createClient

I tested demo sessions locally and I got this error

Copilot

Pull Request Overview

This PR moves the rrweb event stream fetching from an API route to the client and introduces a new ClickHouse proxy for queries. Key changes include:

Updating the ClickHouse client usage and query parameter handling in common-utils and app packages.
Refactoring the event stream logic in sessions to leverage the ClickHouse client with async iterator streaming.
Removing the sessions API route and enhancing the ClickHouse proxy to accept both GET and POST requests with header‐based connection identification.

Reviewed Changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/common-utils/src/clickhouse.ts	Removes connection query parameter and adds header-based handling.
packages/app/src/sessions.ts	Refactors rrweb event streaming to use the ClickHouse client query.
packages/app/pages/api/[...all].ts	Disables automatic body parsing for API requests.
packages/api/src/routers/api/sessions.ts	Removes the sessions API endpoint.
packages/api/src/routers/api/index.ts	Omits sessionsRouter from the API index.
packages/api/src/routers/api/clickhouseProxy.ts	Updates header-based connection validation and extends methods.
packages/api/src/middleware/validation.ts	Adds request header validation middleware.
packages/api/src/api-app.ts	Removes sessions route usage from the API application.
.changeset/neat-badgers-matter.md	Documents the version bump for the updated modules.

Files not reviewed (1)

docker/clickhouse/local/config.xml: Language not supported

Copilot · 2025-04-22T14:40:22Z

packages/app/src/sessions.ts

+              linesFetched++;
+              forFunc(parsed);
+            } catch {
+              // do noting


Typo detected: 'do noting' should be corrected to 'do nothing'.

Suggested change

// do noting

// do nothing

Copilot · 2025-04-22T14:40:23Z

packages/app/src/sessions.ts

+            } catch {
+              // do noting


The catch block silently suppresses JSON parsing errors. Consider logging the error or handling it appropriately to aid in debugging malformed rows.

Suggested change

} catch {

// do noting

} catch (error) {

console.error('Error parsing JSON row:', error, 'Row:', row);

Copilot · 2025-04-22T14:40:23Z

packages/api/src/routers/api/clickhouseProxy.ts

+        // @ts-expect-error _req.query is type ParamQs, which doesn't play nicely with URLSearchParams. TODO: Replace with getting query params from _req.url eventually
        const qparams = new URLSearchParams(_req.query);


Instead of relying on an implicit conversion of _req.query, explicitly converting it by calling toString() or retrieving query params from _req.url would improve clarity and reliability.

Suggested change

// @ts-expect-error _req.query is type ParamQs, which doesn't play nicely with URLSearchParams. TODO: Replace with getting query params from _req.url eventually

const qparams = new URLSearchParams(_req.query);

const url = new URL(_req.url, `http://${_req.headers.host}`);

const qparams = new URLSearchParams(url.search);

wrn14897 · 2025-04-22T21:32:58Z

packages/app/src/sessions.ts

+            },
+          });
+
+          return await clickhouseClient.query({


extra await?

wrn14897

Game changer! Can't wait to get this out 🚢 🚢 🚢

knudtty added 2 commits April 15, 2025 21:08

rrweb streaming from client side

7406d9b

remove qs as dependency; simplify url query params

ca292ba

knudtty commented Apr 16, 2025

View reviewed changes

fix onEvent handling

b5e07d3

knudtty force-pushed the aaron/rrweb-client-side branch from 4883707 to b5e07d3 Compare April 16, 2025 14:53

add changeset

ad66255

MikeShi42 reviewed Apr 16, 2025

View reviewed changes

Merge branch 'v2' into aaron/rrweb-client-side

de10070

vercel bot deployed to Preview April 17, 2025 19:50 View deployment

wrn14897 reviewed Apr 17, 2025

View reviewed changes

packages/app/src/sessions.ts Show resolved Hide resolved

incorporate PR suggestions

68060c8

vercel bot deployed to Preview April 18, 2025 21:33 View deployment

vercel bot had a problem deploying to Preview April 18, 2025 21:46 Failure

username and password available for local mode

6801d96

knudtty force-pushed the aaron/rrweb-client-side branch from 9464f91 to 6801d96 Compare April 21, 2025 14:43

vercel bot deployed to Preview April 21, 2025 14:46 View deployment

wrn14897 reviewed Apr 21, 2025

View reviewed changes

fix local mode and add cors headers to local chdb

4e2e5bc

vercel bot deployed to Preview April 21, 2025 22:16 View deployment

Merge branch 'v2' into aaron/rrweb-client-side

4a2c701

vercel bot deployed to Preview April 21, 2025 22:23 View deployment

Merge branch 'v2' into aaron/rrweb-client-side

fe188f2

vercel bot deployed to Preview April 21, 2025 23:15 View deployment

teeohhem requested a review from Copilot April 22, 2025 14:39

Copilot AI reviewed Apr 22, 2025

View reviewed changes

do not assume chdb cors config; use default client for local mode

5e1bf07

vercel bot deployed to Preview April 22, 2025 18:57 View deployment

Merge remote-tracking branch 'origin/v2' into aaron/rrweb-client-side

2ee94ae

vercel bot deployed to Preview April 22, 2025 19:01 View deployment

wrn14897 reviewed Apr 22, 2025

View reviewed changes

remove extra await

454f7b2

vercel bot deployed to Preview April 22, 2025 21:39 View deployment

wrn14897 approved these changes Apr 22, 2025

View reviewed changes

knudtty merged commit 92a4800 into v2 Apr 22, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755

feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755

knudtty commented Apr 16, 2025

changeset-bot bot commented Apr 16, 2025 •

edited

Loading

knudtty Apr 16, 2025

wrn14897 Apr 17, 2025

knudtty Apr 18, 2025

knudtty Apr 16, 2025

knudtty Apr 16, 2025

wrn14897 Apr 17, 2025

knudtty Apr 18, 2025

knudtty Apr 16, 2025

knudtty Apr 16, 2025

knudtty Apr 16, 2025

MikeShi42 Apr 16, 2025

knudtty Apr 16, 2025

wrn14897 Apr 17, 2025

MikeShi42 Apr 16, 2025

MikeShi42 Apr 16, 2025

knudtty Apr 16, 2025 •

edited

Loading

vercel bot commented Apr 17, 2025 •

edited

Loading

wrn14897 Apr 17, 2025

wrn14897 Apr 17, 2025

knudtty Apr 18, 2025

wrn14897 Apr 17, 2025

knudtty Apr 18, 2025

wrn14897 Apr 21, 2025

knudtty Apr 21, 2025

wrn14897 Apr 21, 2025

wrn14897 Apr 21, 2025

Copilot AI left a comment

Copilot AI Apr 22, 2025

Copilot AI Apr 22, 2025

Copilot AI Apr 22, 2025

wrn14897 Apr 22, 2025

wrn14897 left a comment

	export const getClickhouseClient = () => {
	if (IS_LOCAL_MODE) {
	const localConnections = getLocalConnections();
	if (localConnections.length === 0) {
	console.warn('No local connection found');
	return new ClickhouseClient({
	host: '',
	});
	}
	return new ClickhouseClient({
	host: localConnections[0].host,
	username: localConnections[0].username,
	password: localConnections[0].password,
	});
	}
	return new ClickhouseClient({
	host: PROXY_CLICKHOUSE_HOST,
	});
	};

		// @ts-expect-error _req.query is type ParamQs, which doesn't play nicely with URLSearchParams. TODO: Replace with getting query params from _req.url eventually
		const qparams = new URLSearchParams(_req.query);

feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755

feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755

Conversation

knudtty commented Apr 16, 2025

changeset-bot bot commented Apr 16, 2025 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knudtty Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Apr 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

Copilot AI Apr 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrn14897 left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Apr 16, 2025 •

edited

Loading

knudtty Apr 16, 2025 •

edited

Loading

vercel bot commented Apr 17, 2025 •

edited

Loading