-
Notifications
You must be signed in to change notification settings - Fork 256
feat: move rrweb event stream to client and query through /api/clickhouse-proxy #755
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
Changes from 4 commits
7406d9b
ca292ba
b5e07d3
ad66255
de10070
68060c8
6801d96
4e2e5bc
4a2c701
fe188f2
5e1bf07
2ee94ae
454f7b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
"@hyperdx/common-utils": patch | ||
"@hyperdx/api": patch | ||
"@hyperdx/app": patch | ||
--- | ||
|
||
feat: move rrweb event fetching to the client instead of an api route |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
import express, { Request, Response } from 'express'; | ||
import express, { RequestHandler, Response } from 'express'; | ||
import { createProxyMiddleware } from 'http-proxy-middleware'; | ||
import { z } from 'zod'; | ||
import { validateRequest } from 'zod-express-middleware'; | ||
|
||
import { getConnectionById } from '@/controllers/connection'; | ||
import { getNonNullUserWithTeam } from '@/middleware/auth'; | ||
import { validateRequestHeaders } from '@/utils/validation'; | ||
import { objectIdSchema } from '@/utils/zod'; | ||
|
||
const router = express.Router(); | ||
|
@@ -58,17 +59,23 @@ router.post( | |
}, | ||
); | ||
|
||
router.get( | ||
'/*', | ||
validateRequest({ | ||
query: z.object({ | ||
hyperdx_connection_id: objectIdSchema, | ||
function validation() { | ||
return validateRequestHeaders( | ||
z.object({ | ||
'x-hyperdx-connection-id': objectIdSchema, | ||
}), | ||
}), | ||
async (req, res, next) => { | ||
); | ||
} | ||
|
||
function getConnection(): RequestHandler { | ||
return async (req, res, next) => { | ||
try { | ||
const { teamId } = getNonNullUserWithTeam(req); | ||
const { hyperdx_connection_id } = req.query; | ||
const connection_id = req.headers['x-hyperdx-connection-id']!; // ! because zod already validated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
delete req.headers['x-hyperdx-connection-id']; | ||
const hyperdx_connection_id = Array.isArray(connection_id) | ||
? connection_id.join('') | ||
: connection_id; | ||
|
||
const connection = await getConnectionById( | ||
teamId.toString(), | ||
|
@@ -93,13 +100,15 @@ router.get( | |
console.error('Error fetching connection info:', e); | ||
next(e); | ||
} | ||
}, | ||
createProxyMiddleware({ | ||
}; | ||
} | ||
|
||
function proxyMiddleware(): RequestHandler { | ||
return createProxyMiddleware({ | ||
target: '', // doesn't matter. it should be overridden by the router | ||
changeOrigin: true, | ||
pathFilter: (path, _req) => { | ||
// TODO: allow other methods | ||
return _req.method === 'GET'; | ||
return _req.method === 'GET' || _req.method === 'POST'; | ||
}, | ||
pathRewrite: { | ||
'^/clickhouse-proxy': '', | ||
|
@@ -113,16 +122,23 @@ router.get( | |
on: { | ||
proxyReq: (proxyReq, _req) => { | ||
const newPath = _req.params[0]; | ||
const qparams = new URLSearchParams(_req.query); | ||
qparams.delete('hyperdx_connection_id'); | ||
let qparams = ''; | ||
const qIdx = _req.url.indexOf('?'); | ||
if (qIdx >= 0) { | ||
qparams = _req.url.substring(qIdx); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. _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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will this introduce any encoding issue? I'd suggest to move all irrelevant changes to a separate PR so folks can review it easier There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will do |
||
if (_req._hdx_connection?.username && _req._hdx_connection?.password) { | ||
proxyReq.setHeader( | ||
'X-ClickHouse-User', | ||
_req._hdx_connection.username, | ||
); | ||
proxyReq.setHeader('X-ClickHouse-Key', _req._hdx_connection.password); | ||
} | ||
proxyReq.path = `/${newPath}?${qparams.toString()}`; | ||
if (_req.method === 'POST') { | ||
// TODO: Use fixRequestBody after this issue is resolved: https://github.com/chimurai/http-proxy-middleware/issues/1102 | ||
proxyReq.write(_req.body); | ||
} | ||
proxyReq.path = `/${newPath}${qparams}`; | ||
}, | ||
proxyRes: (proxyRes, _req, res) => { | ||
// since clickhouse v24, the cors headers * will be attached to the response by default | ||
|
@@ -158,7 +174,10 @@ router.get( | |
// ...(config.IS_DEV && { | ||
// logger: console, | ||
// }), | ||
}), | ||
); | ||
}); | ||
} | ||
|
||
router.get('/*', validation(), getConnection(), proxyMiddleware()); | ||
router.post('/*', validation(), getConnection(), proxyMiddleware()); | ||
|
||
export default router; |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import express from 'express'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we should probably put this in the |
||
import { z } from 'zod'; | ||
|
||
export function validateRequestHeaders<T extends z.Schema>(schema: T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically the same as validateRequest function, but they don't validate headers. Maybe should submit a feature request |
||
return function ( | ||
req: express.Request, | ||
res: express.Response, | ||
next: express.NextFunction, | ||
) { | ||
const parsed = schema.safeParse(req.headers); | ||
if (!parsed.success) { | ||
return res.status(400).json({ type: 'Headers', errors: parsed.error }); | ||
} | ||
|
||
return next(); | ||
}; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you elaborate on the decision behind this number? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
}, | ||
}; | ||
|
||
|
@@ -17,12 +18,6 @@ export default (req: NextApiRequest, res: NextApiResponse) => { | |
pathRewrite: { '^/api': '' }, | ||
target: process.env.NEXT_PUBLIC_SERVER_URL || DEFAULT_SERVER_URL, | ||
autoRewrite: true, | ||
/** | ||
* Fix bodyParser | ||
**/ | ||
on: { | ||
proxyReq: fixRequestBody, | ||
}, | ||
// ...(IS_DEV && { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function fixRequestBody does not handle Content-Type: 'text/plain', which makes things horribly annoying. Here's what happens:
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. |
||
// logger: console, | ||
// }), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handling both GET and POST routes now, so broke into RequestHandlers that both can use
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: we don't need to wrap the validate middleware within another function, right? can just be a var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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