feat: add request logs #280

anbraten · 2025-11-04T22:14:19Z

About

What does this PR do?
Add an issue number if this PR can fix or relates to an issue.
Add a changeset if needed with pnpm changeset

Tests

Try to add a test if possible

pkg-pr-new · 2025-11-04T22:14:59Z

npm i https://pkg.pr.new/lagossapp/lagoss/@lagoss/cli@280

commit: 8400172

Copilot

Pull request overview

This pull request migrates the ClickHouse client library from the deprecated clickhouse package to the official @clickhouse/client package, adds comprehensive request logging with additional metadata fields, and renames "function" terminology to "app" throughout the codebase.

Changes:

Migrates from clickhouse (v2.6.0) to @clickhouse/client (v1.12.1) with updated query syntax and response handling
Adds request logging fields: request ID, HTTP method, URL, and response status code to enable better observability
Renames function_id to app_id across the entire codebase (Rust, TypeScript, and database schema) for consistency
Updates Vue components to use typedInject/typedProvide instead of bare inject/provide for better type safety

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
pnpm-lock.yaml	Updates dependencies, replacing deprecated clickhouse package with official @clickhouse/client
packages/dashboard/server/lib/clickhouse.ts	Migrates to new ClickHouse client with connection validation
packages/dashboard/server/api/apps/[appId]/usage.get.ts	Updates query syntax for new client and fixes column references
packages/dashboard/server/api/apps/[appId]/stats.get.ts	Updates query syntax and changes return format from scalar to object
packages/dashboard/server/api/apps/[appId]/logs.get.ts	Updates query with input validation and additional fields
packages/dashboard/package.json	Updates ClickHouse dependency declaration
packages/dashboard/nuxt.config.ts	Changes default database name from 'serverless' to 'lagoss'
packages/dashboard/app/pages/*	Multiple Vue component updates for typed inject/provide pattern
packages/dashboard/app/composables/useInjectProvide.ts	Renames functions to `typedInject` and `typedProvide`
docker-compose.yml	Updates ClickHouse Docker image to official alpine version
crates/serverless/src/serverless.rs	Adds comprehensive request logging with new fields and renames metrics labels
crates/serverless/src/cronjob.rs	Renames function_id to app_id in logging and metrics
crates/serverless/src/clickhouse/mod.rs	Updates data structures with new fields for enhanced logging
crates/serverless/src/clickhouse/migrations.rs	Adds two new migrations for schema updates
crates/serverless/migrations/002_add_missing_fields.sql	Migration to add request ID and HTTP metadata fields
crates/serverless/migrations/003_rename_function_id_to_app_id.sql	Migration to rename function_id column to app_id
crates/runtime_utils/src/response.rs	Refactors response handling to support async logging with response context
crates/cli/src/commands/dev.rs	Updates error messages to use "app" terminology

Files not reviewed (1)

pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-05T06:53:37Z

crates/serverless/src/serverless.rs

+        // TODO: log asset request?
        sender.send_async(run_result).await.unwrap_or(());
    } else if url == FAVICON_URL {
+        // TODO: log favicon request?


The TODO comment should be addressed or tracked properly. Asset requests should be logged to provide complete observability and accurate usage metrics. Consider creating an issue to track this if it won't be implemented immediately.

Suggested change

// TODO: log asset request?

sender.send_async(run_result).await.unwrap_or(());

} else if url == FAVICON_URL {

// TODO: log favicon request?

info!(

deployment = &deployment.id,

asset = asset,

request = request_id,

method = %http_method,

path = %url_string;

"Asset request handled"

);

sender.send_async(run_result).await.unwrap_or(());

} else if url == FAVICON_URL {

info!(

deployment = &deployment.id,

request = request_id,

method = %http_method,

path = %url_string;

"Favicon request handled with 404"

);

Copilot · 2026-02-05T06:53:37Z

crates/serverless/src/cronjob.rs

                                        cpu_time_micros: elapsed.map(|duration| duration.as_micros()),
                                        timestamp,
+                                        http_method: "GET".to_string(), // TODO: use actual cron method
+                                        url: "/".to_string(), // TODO: use actual cron URL


The TODO comment indicates incomplete implementation. Using hardcoded "GET" method and "/" URL for cron jobs prevents accurate tracking of cron job execution. This should be properly implemented to reflect actual cron job identifiers.

Suggested change

url: "/".to_string(), // TODO: use actual cron URL

url: format!("/cron/{}/{}", deployment.function_id, deployment.id), // TODO: use actual cron URL

Copilot · 2026-02-05T06:53:37Z

crates/runtime_utils/src/response.rs

        RunResult::Timeout | RunResult::MemoryLimit => {
-            let event = ResponseEvent::LimitsReached(result);
-            on_event(event).await?;
-
            let body = Full::new(Bytes::from(PAGE_502))
                .map_err(|_| unreachable!())
                .boxed();
-            Ok(Response::builder().status(502).body(body)?)
-        }
-        RunResult::Error(_) => {
-            let event = ResponseEvent::Error(result);
-            on_event(event).await?;
+            let response = Response::builder().status(502).body(body)?;

+            let event = ResponseEvent::MemoryLimit; // TODO: differentiate timeout and memory limit
+            on_event(event, clone_response_without_body(&response)).await?;


The match expression handles both RunResult::Timeout and RunResult::MemoryLimit but always creates ResponseEvent::MemoryLimit. This prevents proper differentiation in the event handlers and could lead to incorrect error categorization. The pattern should be updated to match on the specific result type and create the appropriate ResponseEvent variant (either ResponseEvent::Timeout or ResponseEvent::MemoryLimit).

Copilot · 2026-02-05T06:53:38Z

packages/dashboard/server/api/apps/[appId]/usage.get.ts

+  const result = await clickhouse.query({
+    query: `
+SELECT
  count(*) as requests,
  avg(cpu_time_micros) as cpuTime,
  sum(bytes_in) as bytesIn,
  sum(bytes_out) as bytesOut,
  ${groupBy}(timestamp) AS time
-FROM serverless.requests
+FROM requests
 WHERE
-  function_id = '${app.id}'
+  app_id = '${app.id}'
 AND
-  timestamp >= now() - INTERVAL  ${timeframe === 'Last 24 hours' ? 1 : timeframe === 'Last 7 days' ? 7 : 30} DAY
-GROUP BY time`,
-    )
-    .toPromise()) as { requests: number; cpuTime: number; bytesIn: number; bytesOut: number; time: string }[];
+  timestamp >= now() - INTERVAL ${intervalDays} DAY
+GROUP BY time
+`.trim(),
+    format: 'JSONEachRow',
+  });

-  return result;
+  const rows = await result.json<{
+    requests: number;
+    cpuTime: number;
+    bytesIn: number;
+    bytesOut: number;
+    time: string;
+  }>();
+
+  return rows;


Missing error handling for the query execution. If the query fails or the JSON parsing fails, the error will propagate up without proper context. Consider adding try-catch with meaningful error messages to help with debugging ClickHouse query issues.

packages/dashboard/server/api/apps/[appId]/stats.get.ts

Copilot · 2026-02-05T06:53:40Z

packages/dashboard/server/api/apps/[appId]/stats.get.ts

+FROM requests
 WHERE
-function_id = '${app.id}'
+  app_id = '${app.id}'


SQL injection vulnerability: The query directly interpolates user-controlled input (app.id) into the SQL query string. This should use parameterized queries instead to prevent SQL injection attacks.

Copilot · 2026-02-05T06:53:40Z

crates/serverless/src/cronjob.rs

                                    .0
                                    .write(&RequestRow {
-                                        function_id: deployment.function_id.clone(),
+                                        id: Uuid::new_v4().to_string(), // TODO: use actual request id


The TODO comment should be addressed. Using a randomly generated UUID for cron jobs means they cannot be correlated with actual request tracking. Consider implementing proper request ID tracking for cron jobs to enable better debugging and tracing.

packages/dashboard/server/api/apps/[appId]/logs.get.ts

Copilot · 2026-02-05T06:53:40Z

crates/serverless/src/serverless.rs

+        // TODO: log asset request?
        sender.send_async(run_result).await.unwrap_or(());
    } else if url == FAVICON_URL {
+        // TODO: log favicon request?


The TODO comment should be addressed or tracked properly. Favicon requests should be logged to provide complete observability and accurate usage metrics. Consider creating an issue to track this if it won't be implemented immediately.

Suggested change

// TODO: log favicon request?

increment_counter!(

"lagoss_ignored_requests",

"reason" => "Favicon",

"hostname" => hostname.clone(),

);

info!(req:? = req, hostname = hostname, request = request_id; "Favicon request received; returning 404");

Copilot · 2026-02-05T06:53:41Z

crates/serverless/src/serverless.rs

                .1
                .write(&LogRow {
-                    function_id: log
+                    request_id: String::new(), // TODO: add request ID to logs


The TODO comment indicates incomplete implementation. Logs need to be properly associated with their triggering request for effective debugging and tracing. Consider implementing proper request ID propagation to logs.

anbraten added 2 commits November 3, 2025 18:24

fix: rename inject to avoid conflicts with vue internals

b4d7c00

add request id, status-code, http-method

05b62d9

anbraten and others added 12 commits November 5, 2025 14:09

fix queries

92099fb

reqwest

5e05727

update

d732f01

revert s3 update

cb962c6

Merge remote-tracking branch 'origin/main' into request-logs

195dc7e

Merge remote-tracking branch 'origin/main' into request-logs

ce31158

Merge remote-tracking branch 'origin/main' into request-logs

72220ab

fix

a2b2315

Merge remote-tracking branch 'origin/main' into request-logs

a358c07

fix error handling

2a40284

Merge remote-tracking branch 'origin/main' into request-logs

fa9505b

Merge branch 'main' into request-logs

8400172

anbraten requested a review from Copilot February 5, 2026 06:47

Copilot started reviewing on behalf of anbraten February 5, 2026 06:48 View session

Copilot AI reviewed Feb 5, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add request logs #280

feat: add request logs #280

Uh oh!

anbraten commented Nov 4, 2025

Uh oh!

pkg-pr-new bot commented Nov 4, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Copilot AI Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

-        // TODO: log asset request?
-        sender.send_async(run_result).await.unwrap_or(());
-    } else if url == FAVICON_URL {
-        // TODO: log favicon request?
+        info!(
+            deployment = &deployment.id,
+            asset = asset,
+            request = request_id,
+            method = %http_method,
+            path = %url_string;
+            "Asset request handled"
+        );
+        sender.send_async(run_result).await.unwrap_or(());
+    } else if url == FAVICON_URL {
+        info!(
+            deployment = &deployment.id,
+            request = request_id,
+            method = %http_method,
+            path = %url_string;
+            "Favicon request handled with 404"
+        );

	url: "/".to_string(), // TODO: use actual cron URL
	url: format!("/cron/{}/{}", deployment.function_id, deployment.id), // TODO: use actual cron URL

-        // TODO: log favicon request?
+        increment_counter!(
+            "lagoss_ignored_requests",
+            "reason" => "Favicon",
+            "hostname" => hostname.clone(),
+        );
+        info!(req:? = req, hostname = hostname, request = request_id; "Favicon request received; returning 404");

feat: add request logs #280

Are you sure you want to change the base?

feat: add request logs #280

Uh oh!

Conversation

anbraten commented Nov 4, 2025

About

Tests

Uh oh!

pkg-pr-new bot commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pkg-pr-new bot commented Nov 4, 2025 •

edited

Loading