Skip to content

feat: improve observability #956

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: improve observability #956

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d1e7b5c
feat: add debug logs
vitoUwu Aug 12, 2025
a49aa8c
feat: enrichir contexto de requisição nos logs e atualizar o contexto…
vitoUwu Sep 7, 2025
a07702a
fix: create new object instead of update values
vitoUwu Sep 7, 2025
98adc18
feat: implementar armazenamento de contexto de requisição com AsyncLo…
vitoUwu Sep 7, 2025
ec27f46
refactor: commit cr suggestion
vitoUwu Sep 7, 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
20 changes: 18 additions & 2 deletions blocks/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,16 @@ const wrapLoader = (
new Response(jsonStringEncoded, {
headers: headers,
}),
).catch((error) => logger.error(`loader error ${error}`));
).catch((error) =>
logger.error(`loader error ${error}`, {
loader,
cacheKey: cacheKeyValue,
error: {
message: error.message,
stack: error.stack,
},
})
);
Comment on lines +316 to +325
Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use OTEL exception semantic keys and flatten error attributes

Nested error objects in attributes are non-compliant. Flatten and align with exception.* keys. Also avoid duplicating the error details in both message and attributes if noisy.

Apply:

-          ).catch((error) =>
-            logger.error(`loader error ${error}`, {
-              loader,
-              cacheKey: cacheKeyValue,
-              error: {
-                message: error.message,
-                stack: error.stack,
-              },
-            })
-          );
+          ).catch((error) =>
+            logger.error(`loader cache.put failed`, {
+              loader,
+              cacheKey: cacheKeyValue,
+              "exception.type": error?.name,
+              "exception.message": error?.message,
+              "exception.stacktrace": error?.stack,
+              "event.name": "loader.cache.put.error",
+            })
+          );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
).catch((error) =>
logger.error(`loader error ${error}`, {
loader,
cacheKey: cacheKeyValue,
error: {
message: error.message,
stack: error.stack,
},
})
);
).catch((error) =>
logger.error(`loader cache.put failed`, {
loader,
cacheKey: cacheKeyValue,
"exception.type": error?.name,
"exception.message": error?.message,
"exception.stacktrace": error?.stack,
"event.name": "loader.cache.put.error",
})
);
🤖 Prompt for AI Agents 
In blocks/loader.ts around lines 316 to 325, the current catch logs a nested
error object and duplicates details in the message; replace that with a
flattened set of OTEL exception semantic keys (exception.type,
exception.message, exception.stacktrace) and keep the log message concise (e.g.,
"loader error") while including loader and cacheKey attributes; ensure you
safely derive type/message/stack from the caught value (falling back to
typeof/error.toString() if needed) and do not include a nested error object in
the attributes.


return json;
};
Expand All @@ -336,7 +345,14 @@ const wrapLoader = (
stats.cache.add(1, { status, loader });

callHandlerAndCache().catch((error) =>
logger.error(`loader error ${error}`)
logger.error(`loader error ${error}`, {
loader,
cacheKey: cacheKeyValue,
error: {
message: error.message,
stack: error.stack,
},
})
);
Comment on lines +348 to 356
Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Same here: flatten and standardize exception fields in revalidate path

Match the structure used above for consistency and exporter compatibility.

Apply:

-            callHandlerAndCache().catch((error) =>
-              logger.error(`loader error ${error}`, {
-                loader,
-                cacheKey: cacheKeyValue,
-                error: {
-                  message: error.message,
-                  stack: error.stack,
-                },
-              })
-            );
+            callHandlerAndCache().catch((error) =>
+              logger.error(`loader revalidate failed`, {
+                loader,
+                cacheKey: cacheKeyValue,
+                "exception.type": error?.name,
+                "exception.message": error?.message,
+                "exception.stacktrace": error?.stack,
+                "event.name": "loader.revalidate.error",
+              })
+            );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logger.error(`loader error ${error}`, {
loader,
cacheKey: cacheKeyValue,
error: {
message: error.message,
stack: error.stack,
},
})
);
callHandlerAndCache().catch((error) =>
logger.error(`loader revalidate failed`, {
loader,
cacheKey: cacheKeyValue,
"exception.type": error?.name,
"exception.message": error?.message,
"exception.stacktrace": error?.stack,
"event.name": "loader.revalidate.error",
})
);
🤖 Prompt for AI Agents 
In blocks/loader.ts around lines 348 to 356, the revalidate-path logger call
nests the error under an "error" object which differs from the standardized,
flattened exception shape used elsewhere; change the logger.error invocation to
use the same flattened fields (e.g.,
exception_name/exception_message/exception_stack or name/message/stack depending
on the project's convention) alongside loader and cacheKey, and remove the
nested error object so the payload matches the other logs and exporter
expectations.

} else {
status = "hit";
Expand Down
10 changes: 10 additions & 0 deletions deco.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,16 @@ export type RequestContext = {
/** Cancelation token used for early processing halt */
signal?: AbortSignal;
framework?: "fresh" | "htmx";
/** Request URL for context */
url?: string;
/** Request method */
method?: string;
/** Request pathname */
pathname?: string;
/** User agent */
userAgent?: string;
/** Correlation ID for tracing */
correlationId?: string;
};

export type WellKnownHostingPlatform =
Expand Down
34 changes: 34 additions & 0 deletions observability/otel/logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as log from "@std/log";

import { type LevelName, LogLevels } from "@std/log/levels";
import type { LogRecord } from "@std/log/logger";
import { Context } from "../../deco.ts";

import {
type Attributes,
Expand Down Expand Up @@ -133,6 +134,9 @@ export class OpenTelemetryHandler extends log.BaseHandler {

const otelSeverityNumber = this.toOtelSeverityNumber(logRecord.level);

// Automatically enrich logs with request context
const requestContext = this.extractRequestContext();

this._logger?.emit({
severityNumber: otelSeverityNumber,
severityText: OTEL_SEVERITY_NAME_MAP[otelSeverityNumber] ??
Expand All @@ -141,10 +145,40 @@ export class OpenTelemetryHandler extends log.BaseHandler {
attributes: {
...typeof firstArg === "object" ? firstArg : {},
loggerName: logRecord.loggerName,
...requestContext,
},
});
}

/**
* Extracts request context data from the current context
*/
private extractRequestContext(): Record<string, unknown> {
try {
const activeContext = Context.active();
const requestContext = activeContext.request;

return {
siteId: activeContext.siteId,
siteName: activeContext.site,
platform: activeContext.platform,
deploymentId: activeContext.deploymentId,
request: {
url: requestContext?.url,
method: requestContext?.method,
pathname: requestContext?.pathname,
userAgent: requestContext?.userAgent,
correlationId: requestContext?.correlationId,
framework: requestContext?.framework,
},
};
} catch {
console.error("Error extracting request context");
// If context is not available, return empty object
return {};
}
}

Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Return flat, spec-aligned keys (and default framework) from extractRequestContext

Use dot-delimited keys and OTEL-ish names; avoid nested objects. Also default framework to "fresh".

Apply:

-  private extractRequestContext(): Record<string, unknown> {
+  private extractRequestContext(): Record<string, unknown> {
     try {
       const activeContext = Context.active();
-      const requestContext = activeContext.request;
-
-      return {
-        siteId: activeContext.siteId,
-        siteName: activeContext.site,
-        platform: activeContext.platform,
-        deploymentId: activeContext.deploymentId,
-        request: {
-          url: requestContext?.url,
-          method: requestContext?.method,
-          pathname: requestContext?.pathname,
-          userAgent: requestContext?.userAgent,
-          correlationId: requestContext?.correlationId,
-          framework: requestContext?.framework,
-        },
-      };
+      const r = activeContext.request;
+      return {
+        "site.id": activeContext.siteId,
+        "site.name": activeContext.site,
+        "host.platform": activeContext.platform,
+        "deployment.id": activeContext.deploymentId,
+        "request.url": r?.url,
+        "request.method": r?.method,
+        "request.pathname": r?.pathname,
+        "user_agent.original": r?.userAgent,
+        "request.correlation_id": r?.correlationId,
+        "framework.name": r?.framework ?? "fresh",
+      };
     } catch {
       console.error("Error extracting request context");
       // If context is not available, return empty object
       return {};
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Extracts request context data from the current context
*/
private extractRequestContext(): Record<string, unknown> {
try {
const activeContext = Context.active();
const requestContext = activeContext.request;
return {
siteId: activeContext.siteId,
siteName: activeContext.site,
platform: activeContext.platform,
deploymentId: activeContext.deploymentId,
request: {
url: requestContext?.url,
method: requestContext?.method,
pathname: requestContext?.pathname,
userAgent: requestContext?.userAgent,
correlationId: requestContext?.correlationId,
framework: requestContext?.framework,
},
};
} catch {
console.error("Error extracting request context");
// If context is not available, return empty object
return {};
}
}
private extractRequestContext(): Record<string, unknown> {
try {
const activeContext = Context.active();
const r = activeContext.request;
return {
"site.id": activeContext.siteId,
"site.name": activeContext.site,
"host.platform": activeContext.platform,
"deployment.id": activeContext.deploymentId,
"request.url": r?.url,
"request.method": r?.method,
"request.pathname": r?.pathname,
"user_agent.original": r?.userAgent,
"request.correlation_id": r?.correlationId,
"framework.name": r?.framework ?? "fresh",
};
} catch {
console.error("Error extracting request context");
// If context is not available, return empty object
return {};
}
}
🤖 Prompt for AI Agents 
In observability/otel/logger.ts around lines 153-181, extractRequestContext
currently returns a nested request object and non-spec keys; change it to return
a flat Record<string, unknown> using dot-delimited, OTEL-ish keys (no nested
objects) and default framework to "fresh" when missing. Specifically, map
context values to keys like "site.id", "site.name", "platform", "deployment.id",
"http.url", "http.method", "http.target" (or "http.pathname"),
"http.user_agent", "correlation.id" (or "trace.correlation_id"), and "framework"
(defaulting to "fresh"); ensure safe optional access to requestContext
properties and keep the try/catch behavior returning {} on error.

log(_msg: string) {}
flush() {}

Expand Down
13 changes: 12 additions & 1 deletion runtime/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,13 +222,24 @@ export class Deco<TAppManifest extends AppManifest = AppManifest> {

const liveContext = this.ctx;
const request = forceHttps(req);
const url = new URL(request.url);

// Create a new request context
liveContext.request = {};

liveContext.request.url = request.url;
liveContext.request.method = request.method;
liveContext.request.pathname = url.pathname;
liveContext.request.userAgent = request.headers.get("user-agent") ||
undefined;
liveContext.request.correlationId = correlationId;

Comment on lines 223 to 236
Copy link

@coderabbitai coderabbitai bot Sep 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid mutating the shared context; use Context.active() to prevent cross-request leaks

this.ctx is the global/default context. Mutating liveContext.request here risks leaking request data across concurrent requests if AsyncLocalStorage isn’t bound with a per-request clone. Bind per-request context and/or read the active one before mutation.

Apply:

-    const liveContext = this.ctx;
+    const liveContext = Context.active();

And set the request context atomically to reduce intermediate inconsistent states:

-    // Create a new request context
-    liveContext.request = {};
-
-    liveContext.request.url = request.url;
-    liveContext.request.method = request.method;
-    liveContext.request.pathname = url.pathname;
-    liveContext.request.userAgent = request.headers.get("user-agent") ||
-      undefined;
-    liveContext.request.correlationId = correlationId;
+    liveContext.request = {
+      url: request.url,
+      method: request.method,
+      pathname: url.pathname,
+      userAgent: request.headers.get("user-agent") ?? undefined,
+      correlationId,
+    };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const liveContext = this.ctx;
const request = forceHttps(req);
const url = new URL(request.url);
// Create a new request context
liveContext.request = {};
liveContext.request.url = request.url;
liveContext.request.method = request.method;
liveContext.request.pathname = url.pathname;
liveContext.request.userAgent = request.headers.get("user-agent") ||
undefined;
liveContext.request.correlationId = correlationId;
const liveContext = Context.active();
const request = forceHttps(req);
const url = new URL(request.url);
liveContext.request = {
url: request.url,
method: request.method,
pathname: url.pathname,
userAgent: request.headers.get("user-agent") ?? undefined,
correlationId,
};
🤖 Prompt for AI Agents 
In runtime/mod.ts around lines 223 to 236, you are mutating the shared this.ctx
which can leak data across concurrent requests; instead obtain the per-request
context via Context.active() (or clone this.ctx into a fresh context tied to the
current request) and assign the request object in one atomic operation. Replace
the stepwise property assignments with creating a new request object (containing
url, method, pathname, userAgent, correlationId) and set it onto the per-request
context returned by Context.active() (or the cloned context) in a single
assignment so no intermediate mutated state is exposed to other requests.

state.release = liveContext.release!;
const response = {
headers: new Headers(defaultHeaders),
status: undefined,
};
state.url = new URL(request.url);
state.url = url;
state.response = response;
state.bag = new WeakMap();
state.vary = vary();
Expand Down
Loading