Skip to content

feat(webui): Add logs to presto search routes. #1275

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

feat(webui): Add logs to presto search routes. #1275

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7bfcc55
latest
Aug 28, 2025
4d1be9d
Merge branch 'main' into logs
hoophalab Aug 29, 2025
bc1bbd7
Merge branch 'main' into logs
hoophalab Sep 2, 2025
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
4 changes: 4 additions & 0 deletions components/webui/server/src/routes/api/presto-search/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
// eslint-disable-next-line max-lines-per-function
async (request, reply) => {
const {queryString} = request.body;
request.log.info({queryString}, "/api/presto-search/query args");

Comment on lines 62 to 64
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 28, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Avoid logging full query text; truncate/redact and align message prefix.

Raw queries can contain PII/secrets and can bloat logs. Truncate before logging (and consider gating behind a debug level/feature flag). Also align message prefix with other routes (no leading slash).

 const {queryString} = request.body;
-request.log.info({queryString}, "/api/presto-search/query args");
+const maxLen = 512;
+const qsLog =
+  ("string" === typeof queryString && queryString.length > maxLen)
+    ? `${queryString.slice(0, maxLen)}…(truncated)`
+    : queryString;
+request.log.info({queryString: qsLog}, "api/presto-search/query args");

Please confirm this logging aligns with your privacy policy and production log size constraints.

🏁 Script executed:

#!/bin/bash
set -e
# Search for all logging calls in API routes to compare prefixes
rg -n -C2 'request\.log\.' components/webui/server/src/routes/api/**/*.ts

Length of output: 12144

Truncate queryString and log at debug level

  • Truncate queryString (>512 chars) to ${queryString.slice(0,512)}…(truncated) to avoid PII/excessive logs
  • Switch to request.log.debug({ queryString: qsLog }, "/api/presto-search/query args") (debug-level, matching /api/search/query args)

Please confirm this meets our privacy policy and production log-size constraints.

🤖 Prompt for AI Agents 
In components/webui/server/src/routes/api/presto-search/index.ts around lines 62
to 64, the handler currently logs the full queryString at info level; change
this to truncate any queryString longer than 512 characters to
`${queryString.slice(0,512)}…(truncated)` (or equivalent) into a local variable
(e.g., qsLog) and log that at debug level via request.log.debug({ queryString:
qsLog }, "/api/presto-search/query args") to avoid PII/excessive logs and match
the /api/search/query args logging level.

let searchJobId: string;
let totalResultsCount = 0;
Expand Down Expand Up @@ -213,6 +214,9 @@ const plugin: FastifyPluginAsyncTypebox = async (fastify) => {
},
async (request, reply) => {
const {searchJobId} = request.body;

request.log.info({searchJobId}, "api/presto-search/cancel args");
Comment on lines +217 to +218
Copy link
Member

@junhaoliao junhaoliao Sep 2, 2025

Choose a reason for hiding this comment

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

Suggested change
request.log.info({searchJobId}, "api/presto-search/cancel args");
request.log.info({searchJobId}, "/api/presto-search/cancel args");


Comment on lines 216 to +219
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Keep log message prefixes consistent across endpoints.

This uses "api/presto-search/cancel args" (no leading slash) while the /query log uses a leading slash. Favour the no-slash form across all three endpoints for consistency.

🤖 Prompt for AI Agents 
In components/webui/server/src/routes/api/presto-search/index.ts around lines
216 to 219, ensure log-message prefixes are consistent by using the
no-leading-slash form for all endpoints; find any request.log.* calls that use
paths like "/api/presto-search/..." (e.g. the /query endpoint) and remove the
leading slash so they match "api/presto-search/...", keeping the same
descriptive suffix (e.g. "args", "start", "cancel") and leaving the existing
structured meta (like {searchJobId}) unchanged.

await new Promise<void>((resolve, reject) => {
Presto.client.kill(searchJobId, (error) => {
if (null !== error) {
Expand Down
Loading