feat: improve observability #956
Conversation
WalkthroughAdds per-request metadata propagation via AsyncLocalStorage, extends RequestContext with optional request fields, enriches OpenTelemetry logs with that context, updates loader cache error logging to structured objects, and reuses a single URL instance during request handling. No control-flow or return-value changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Middleware as runtime/middleware.ts
participant Runtime as runtime/mod.ts
participant ALS as observability/otel/context.ts
participant Handler as downstream handlers
Client->>Middleware: HTTP request
Middleware->>Runtime: build state & liveContext
Runtime->>ALS: withRequestContext({url,method,pathname,userAgent,correlationId}, fn)
note right of ALS #e6f7ff: AsyncLocalStorage.run sets store
ALS->>Handler: invoke next() (context available)
Handler->>Handler: async operations (can access request context)
sequenceDiagram
autonumber
participant Logger as observability/otel/logger.ts
participant ALS as observability/otel/context.ts
participant Sink as Log Sink
Logger->>ALS: requestContextStore.getStore() / Context.active()
alt context present
Logger->>Logger: merge request.* fields into attrs
else
Logger->>Logger: continue without request attrs
end
Logger->>Sink: emit(log, mergedAttrs)
sequenceDiagram
autonumber
participant Loader as blocks/loader.ts
participant Cache as Cache
participant Logger as observability/otel/logger.ts
Loader->>Cache: cache.put(...)
Cache-->>Loader: reject(error)
Loader->>Logger: log({ loader, cacheKey, error: { message, stack } }, text)
Loader->>Loader: stale-while-revalidate -> callHandlerAndCache()
Loader-->>Loader: catch(error)
Loader->>Logger: log({ loader, cacheKey, error: { message, stack } }, text)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
Tagging OptionsShould a new tag be published when this PR is merged?
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
runtime/mod.ts (1)
171-199: Wrap the HTTP handler with a per‐request Context.bind
Inruntime/handler.tsx(around line 173), instead of returninghono.fetch.bind(hono)directly, wrap it withContext.bind(orRequestContext.bind) and include a newrequestobject (at minimumsignal,framework,url,method,pathname,userAgent) so thatContext.active()is isolated per incoming request.observability/otel/logger.ts (1)
137-149: Flatten attributes; nested objects are not valid OTEL attribute valuesAttributes should be flat primitives/arrays. The current request: { … } object risks being dropped or rejected by exporters.
Apply:
- // Automatically enrich logs with request context - const requestContext = this.extractRequestContext(); + // Automatically enrich logs with request context (flat attributes) + const requestContext = this.extractRequestContext();And ensure extractRequestContext returns flat keys (see below).
🧹 Nitpick comments (2)
runtime/mod.ts (1)
230-235: Consider redacting URL query/PII before storing it in contextStoring the full URL may capture tokens/emails in query strings. Prefer pathname or a redacted URL.
Example:
- url: request.url, + url: `${url.origin}${url.pathname}`, // no query/hashdeco.ts (1)
31-41: Additions look good; type nits for stronger contractsOptional request fields are fine. Minor type tightenings:
Apply:
- /** Request URL for context */ - url?: string; - /** Request method */ - method?: string; - /** Request pathname */ - pathname?: string; + /** Full (possibly redacted) request URL */ + url?: string; + /** Request method */ + method?: Request["method"]; + /** Request pathname (no query/hash) */ + pathname?: string;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
blocks/loader.ts(2 hunks)deco.ts(1 hunks)observability/otel/logger.ts(3 hunks)runtime/mod.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
runtime/mod.ts (2)
runtime/middleware.ts (1)
state(40-42)utils/http.ts (1)
defaultHeaders(142-144)
blocks/loader.ts (2)
observability/otel/config.ts (1)
logger(60-70)observability/mod.ts (1)
logger(2-2)
observability/otel/logger.ts (1)
deco.ts (1)
Context(119-141)
🔇 Additional comments (1)
runtime/mod.ts (1)
242-242: LGTM: reuse a single URL objectReusing url via state.url avoids repeated parsing and is a nice micro-optimization.
| ).catch((error) => | ||
| logger.error(`loader error ${error}`, { | ||
| loader, | ||
| cacheKey: cacheKeyValue, | ||
| error: { | ||
| message: error.message, | ||
| stack: error.stack, | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🛠️ 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.
| ).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.
| logger.error(`loader error ${error}`, { | ||
| loader, | ||
| cacheKey: cacheKeyValue, | ||
| error: { | ||
| message: error.message, | ||
| stack: error.stack, | ||
| }, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🛠️ 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.
| 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.
| /** | ||
| * 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 {}; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
| /** | |
| * 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.
| 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; | ||
|
|
There was a problem hiding this comment.
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.
| 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.
…calStorage e atualizar logger para extrair dados do novo contexto
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
observability/otel/logger.ts (1)
154-174: Adopt OTEL-ish, flat attribute keys (align with spans) and default framework.Keys like
request.userAgent/request.pathnameare non-standard and diverge from span attributes (http.request.*,url.path,user_agent.original). Aligning improves cross-signal correlation.- private extractRequestContext(): Record<string, unknown> { + private extractRequestContext(): Record<string, unknown> { try { - const context = requestContextStore.getStore() || - Context.active()?.request; - - return { - "request.url": context?.url, - "request.method": context?.method, - "request.pathname": context?.pathname, - "request.userAgent": context?.userAgent, - "request.correlationId": context?.correlationId, - }; + const r = requestContextStore.getStore() || Context.active()?.request; + return { + "http.request.url": r?.url, + "http.request.method": r?.method, + "url.path": r?.pathname, + "user_agent.original": r?.userAgent, + "request.correlation_id": r?.correlationId, + "framework.name": "fresh", + }; } catch (error) { console.error("Error extracting request context:", error); // If context is not available, return empty object return {}; } }
🧹 Nitpick comments (1)
observability/otel/context.ts (1)
7-13: Export the interface and consider future-proofing fields.Exporting
RequestContextlets callers type their payloads; consider addingframework?: stringif you intend to log it.-interface RequestContext { +export interface RequestContext { url?: string; method?: string; pathname?: string; userAgent?: string; correlationId?: string; + // framework?: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
observability/otel/context.ts(1 hunks)observability/otel/logger.ts(4 hunks)runtime/middleware.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
runtime/middleware.ts (1)
observability/otel/context.ts (1)
withRequestContext(17-22)
observability/otel/logger.ts (2)
deco.ts (2)
context(188-188)Context(119-141)observability/otel/context.ts (1)
requestContextStore(15-15)
observability/otel/context.ts (1)
deps.ts (2)
createContextKey(12-12)context(11-11)
🔇 Additional comments (7)
runtime/middleware.ts (1)
6-6: Import looks correct.
withRequestContextis used below; no issues.observability/otel/logger.ts (3)
6-6: Import is appropriate.Using
requestContextStorekeeps log enrichment decoupled fromDeco.Context.
26-26: Good fallback toContext.active().Keeps compatibility when ALS isn’t set.
138-150: Enrichment placement LGTM.Merging extracted request attrs into
attributesis correct. No functional concerns here.observability/otel/context.ts (3)
15-15: Store definition LGTM.Type aligns with the interface; no issues.
17-22: Wrapper LGTM.
withRequestContextcorrectly delegates torunand supports async fns via returned Promise.
1-1: Guardnode:async_hooksimport or enable Deno --compat
Deno tasks don’t include--compat, soimport { AsyncLocalStorage } from "node:async_hooks"will fail in pure Deno; either run your CI/dev commands with--compat(or add it in deno.json tasks) or switch tostd/node/async_hooksand provide a no-op fallback for non-Node runtimes.
Summary by CodeRabbit