feat: implement unified structured logging system with OpenTelemetry integration and correlation ID support #197
Conversation
…integration and correlation ID support
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a structured logging subsystem (core, server, client) with correlation ID utilities, integrates it across tRPC, server services, and instrumentation, replaces console logs with structured logger calls, and adds test mocks plus a React hook for logger consumption. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ClientLogger as Client Logger
participant tRPC as tRPC Handler
participant ServerLogger as Server Logger
participant PostHog
Browser->>ClientLogger: getOrGenerateClientCorrelationId()
ClientLogger-->>Browser: return/persist correlationId
Browser->>tRPC: HTTP request (may include x-correlation-id)
tRPC->>ServerLogger: createContext(req) → correlationId
ServerLogger->>ServerLogger: setCorrelationId (AsyncLocalStorage)
Note over ServerLogger,tRPC: Request processing uses correlation context
tRPC->>ServerLogger: logger.debug/info/warn/error(..., { correlationId, ... })
alt error at server
ServerLogger->>PostHog: dynamic import -> trackException({ correlationId, context })
end
tRPC->>Browser: response with x-correlation-id header
Browser->>ClientLogger: receive header, persist if needed
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #197 +/- ##
==========================================
+ Coverage 70.42% 71.10% +0.67%
==========================================
Files 110 113 +3
Lines 2840 2924 +84
Branches 861 865 +4
==========================================
+ Hits 2000 2079 +79
- Misses 490 494 +4
- Partials 350 351 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
lib/logger/correlation.ts (1)
78-96: Cookie parsing could be more defensive.Line 86 uses array destructuring which could fail silently if a cookie doesn't contain an "=" character. While unlikely in practice, the code could be more robust.
Consider this more defensive approach:
try { const cookies = document.cookie.split(";"); for (const cookie of cookies) { - const [name, value] = cookie.trim().split("="); + const parts = cookie.trim().split("="); + const name = parts[0]; + const value = parts.slice(1).join("="); // Handle values containing "=" if (name === CORRELATION_ID_COOKIE && value) { return decodeURIComponent(value); } } } catch { // Cookie parsing failed }This also correctly handles cookie values that contain "=" characters.
hooks/use-logger.ts (1)
1-38:useLoggerhook is sound; consider stability ofbaseContextThe hook correctly returns the singleton logger when no context is passed and a child logger when
baseContextis provided. Only minor note: sincebaseContextis in the dependency array, callers should avoid creating it inline each render if they care about preserving the same logger instance, e.g. memoize the context or define it outside render.app/api/trpc/[trpc]/route.ts (1)
2-32: TRPC handler changes correctly wire correlation IDs and centralized error loggingMaking
handlerasync, precomputingctx = await createContext({ req }), passing it viacreateContext: () => ctx, and addingresponseMetawith[CORRELATION_ID_HEADER]: ctx.correlationIdis consistent with the new correlation model. The dynamiclogger.errorinonErroralso matches the Logger API and keeps unhandled TRPC errors going through the structured logger.lib/logger/index.test.ts (1)
1-132: Logger core test coverage is solid; minor note onNODE_ENVassumptionThe tests exercise all key core helpers (level filtering, context sanitization, error serialization, log entry creation, and JSON formatting) and match the current implementations well. One small consideration: the
getMinLogLevel()expectation of"warn"depends onNODE_ENV === "test", so ensure your Vitest setup pins that value to avoid surprises if the test runner’s environment configuration changes.trpc/init.ts (1)
7-9: TRPC context + middleware logging are wired correctly; be intentional aboutinputloggingThe new context creation (
{ req, ip, correlationId }plussetCorrelationId) cleanly integrates correlation IDs into AsyncLocalStorage and matches how the server logger expects to pick them up. ThewithLoggingandwithDomainAccessUpdatemiddlewares correctly use the server logger, with structured fields for path/type/duration and domain.One thing to keep an eye on: spreading
inputinto the log context is convenient, but it means any keys that are considered “safe” bysanitizeContextcould be logged for all procedures. As long asSAFE_FIELDSin the logger core remains narrowly scoped to non-PII keys (likedomain,path, etc.), this is fine; otherwise, consider restricting the logged input fields per-procedure instead of logging the full input object.Also applies to: 32-48, 64-78, 84-90, 106-111, 132-135
lib/logger/client.ts (1)
1-205: Client logger implementation aligns with the shared core and is robust against failuresThe
ClientLoggerclass and the exportedlogger/createLoggerhelpers correctly implement theLoggerinterface, respectshouldLog/min-level filtering, attach the client correlation ID to log entries, and guard all paths with try/catch so logging can’t crash the app. Error/fatal paths correctly always emit toconsole.errorand conditionally forward to PostHog via a dynamic analytics import.Two minor nits:
- The JSDoc mentions “Browser context (user agent, viewport)”, but those fields are not currently added to the log context or analytics payloads; either wiring them in or trimming the claim would avoid confusion.
- As with the server-side logger, the actual PII-safety hinges on the
sanitizeContext/SAFE_FIELDSconfiguration in the core; given you’re merging arbitrarybaseContext+ per-callcontext, it’s worth keeping that allowlist intentionally narrow.lib/logger/server.ts (2)
29-33: Confirm this logger is only used in Node runtime (AsyncLocalStorage constraint)
AsyncLocalStoragefromnode:async_hooksisn’t available in the Edge runtime. If any route/component that can run in Edge imports@/lib/logger/server, you’ll get runtime failures on Vercel.It’s worth double‑checking that:
- All imports of
@/lib/logger/serverare in code explicitly configured for the Node.js runtime, and- Edge-compatible code paths use the client logger or a separate edge-safe logger.
237-248: Examples use context fields that are currently dropped bysanitizeContextThe JSDoc examples use:
logger.info("User logged in", { userId: "123" });logger.error("Database connection failed", error, { table: "users" });However,
sanitizeContextinlib/logger/index.tsonly allows keys fromSAFE_FIELDS, which currently does not includeuserIdortable. Those context entries will be stripped from the final log entries.If that’s intentional for PII reasons, consider adjusting the examples to only use allowed, non‑PII keys (e.g.
component,action,statusCode) so they match real behavior. Otherwise, you may want to extendSAFE_FIELDSaccordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
AGENTS.md(3 hunks)app/api/trpc/[trpc]/route.ts(1 hunks)hooks/use-logger.ts(1 hunks)instrumentation.ts(1 hunks)lib/logger/client.ts(1 hunks)lib/logger/correlation.ts(1 hunks)lib/logger/index.test.ts(1 hunks)lib/logger/index.ts(1 hunks)lib/logger/server.ts(1 hunks)server/services/dns.ts(7 hunks)server/services/headers.test.ts(2 hunks)server/services/headers.ts(5 hunks)server/services/hosting.ts(6 hunks)server/services/registration.test.ts(3 hunks)server/services/registration.ts(6 hunks)trpc/init.ts(5 hunks)vitest.setup.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
instrumentation.ts (2)
lib/logger/client.ts (1)
logger(178-178)lib/logger/server.ts (1)
logger(249-249)
server/services/headers.ts (2)
lib/logger/client.ts (2)
logger(178-178)createLogger(190-205)lib/logger/server.ts (2)
logger(249-249)createLogger(261-276)
hooks/use-logger.ts (3)
lib/logger/index.ts (2)
LogContext(14-14)Logger(35-42)lib/logger/client.ts (1)
createLogger(190-205)lib/logger/server.ts (1)
createLogger(261-276)
lib/logger/server.ts (2)
lib/logger/client.ts (4)
trace(137-139)error(153-155)logger(178-178)createLogger(190-205)lib/logger/index.ts (6)
Logger(35-42)LogLevel(12-12)LogContext(14-14)shouldLog(104-109)createLogEntry(169-214)formatLogEntry(162-164)
trpc/init.ts (2)
lib/logger/correlation.ts (1)
getOrGenerateCorrelationId(33-36)lib/logger/server.ts (2)
setCorrelationId(38-40)logger(249-249)
lib/logger/client.ts (3)
lib/logger/index.ts (6)
Logger(35-42)LogLevel(12-12)LogContext(14-14)shouldLog(104-109)createLogEntry(169-214)formatLogEntry(162-164)lib/logger/correlation.ts (1)
getOrGenerateClientCorrelationId(102-121)lib/logger/server.ts (3)
error(224-226)logger(249-249)createLogger(261-276)
lib/logger/index.ts (2)
lib/logger/client.ts (1)
error(153-155)lib/logger/server.ts (1)
error(224-226)
server/services/dns.ts (2)
lib/logger/client.ts (3)
logger(178-178)createLogger(190-205)error(153-155)lib/logger/server.ts (3)
logger(249-249)createLogger(261-276)error(224-226)
server/services/registration.ts (2)
lib/logger/client.ts (3)
logger(178-178)createLogger(190-205)error(153-155)lib/logger/server.ts (3)
logger(249-249)createLogger(261-276)error(224-226)
lib/logger/index.test.ts (1)
lib/logger/index.ts (6)
shouldLog(104-109)getMinLogLevel(91-99)sanitizeContext(115-137)serializeError(142-157)createLogEntry(169-214)formatLogEntry(162-164)
🔇 Additional comments (13)
server/services/dns.ts (3)
13-13: LGTM: Structured logger integration.The logger is properly initialized with source context, enabling consistent structured logging across the DNS service.
Also applies to: 23-23
87-87: LGTM: Comprehensive logging coverage.The logging statements provide good observability with structured context including domain, resolver, counts, and timing information. The data appears PII-safe as domain names are public information.
Also applies to: 173-177, 287-292, 394-399
251-256: Verify error suppression is intentional.The error parameter is named
_errin catch blocks without logging the error details. While this reduces log noise for expected failures (scheduling errors), ensure this aligns with debugging requirements.If these are truly fire-and-forget operations where failure details aren't needed, this pattern is acceptable. Otherwise, consider logging the error message at minimum:
- ).catch((_err) => { + ).catch((err) => { logger.warn(`schedule failed partial ${registrable}`, { domain: registrable, type: "partial", + error: err instanceof Error ? err.message : String(err), }); });Also applies to: 297-301, 386-390
lib/logger/correlation.ts (1)
102-121: LGTM: Well-structured client-side correlation ID management.The priority order (cookie > localStorage > generate) is logical and the implementation includes proper SSR guards and error handling.
server/services/headers.test.ts (1)
168-170: LGTM: Clean test migration to integration-level logging verification.The removal of console spies in favor of global logger mocks (via vitest.setup.ts) is a cleaner approach that reduces test boilerplate while maintaining observability through integration tests.
Also applies to: 189-190
server/services/registration.test.ts (1)
236-237: LGTM: Consistent test migration strategy.The approach matches headers.test.ts, maintaining consistency across the test suite while reducing per-test boilerplate.
Also applies to: 259-260, 280-281
server/services/registration.ts (3)
10-10: LGTM: Structured logger integration.Logger properly initialized with source context for the registration service.
Also applies to: 17-17
58-58: LGTM: Comprehensive logging with appropriate context.The logging statements provide good coverage across the registration flow with structured context including domain, registrar information, and registration status. The context data is PII-safe.
Also applies to: 135-140, 159-162, 188-191, 314-318
128-132: Verify error suppression pattern consistency.Similar to the DNS service, scheduling errors use the
_errpattern to suppress error details. This is consistent with the DNS service but ensure this aligns with debugging requirements across the codebase.See the DNS service review comment for the same pattern. Consider documenting this convention if it's intentional across all services.
Also applies to: 307-311
vitest.setup.ts (1)
25-66: LGTM: Comprehensive logger mocks with minor note.The logger mocks are well-structured and cover both server and client implementations. The inclusion of correlation ID utilities in the server mock is appropriate.
Minor note: Line 45's
withCorrelationIdmock executes the function synchronously. If any tests rely on correlation ID propagation through async context, this simplified mock might not catch issues. However, this is likely acceptable for unit tests where the full AsyncLocalStorage behavior isn't needed.instrumentation.ts (1)
15-24: PostHog error tracking is properly maintained.Verification confirms the call chain:
logger.error()→logWithError()→trackErrorInPostHog()→analytics.trackException(). Both client (lib/logger/client.ts:123) and server (lib/logger/server.ts:194) implementations correctly route errors to PostHog, with the server implementation usingafter()for non-blocking tracking as documented. The change preserves observability.server/services/hosting.ts (1)
17-31: Logging integration indetectHostinglooks consistent and non-invasiveThe new
createLogger({ source: "hosting" })usage and added debug/info/warn/error calls correctly follow the logger API, keep existing control flow intact, and log only non-PII fields (domain + provider names). This should significantly improve observability without behavioral regressions.Also applies to: 42-42, 106-112, 129-130, 281-292
server/services/headers.ts (1)
12-12: Headers service logging refactor is correct and preserves behaviorThe move to
createLogger({ source: "headers" })with structured debug/info/warn/error calls cleanly replaces the previous console usage without altering control flow. Context payloads (domain/status/count/cached) are appropriate for logging and should pass through the sanitizer safely.Also applies to: 17-18, 30-30, 68-73, 112-115, 119-123, 140-145
…t handling in log entries
…ization and prevent unnecessary logger recreation
…ror for automatic exception tracking
…r improved error handling in error boundaries
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/logger/server.ts (1)
84-232: Server logger implementation is robust; a couple of small optional cleanupsThe level filtering, structured entry creation, console routing, and PostHog tracking are all wired correctly and defensively (logging never throws, analytics failures are swallowed). A few optional polish points:
- The constructor’s
parseLogLevel(process.env.LOG_LEVEL) || NODE_ENVchain duplicates logic that likely already lives in the core helpers (e.g.,getMinLogLevel()inlib/logger/index.ts). If you still want a single source of truth for env parsing, consider delegating to that helper instead of hand-rolling the fallback here.logWithErrorhas a default branch that logs viaconsole.logfor non-error/fatallevels, but today it’s only called for those two levels. If you ever extend it to other levels, you might want to mirror theconsole.debug/info/warnrouting fromlog()or just delegate tolog()to keep behavior consistent.These are non-blocking and the current behavior is correct.
Also applies to: 238-277
AGENTS.md (1)
12-12: Deduplicate the “Structured Logging” sectionThe new structured logging docs (project structure bullet, test mocking note, and the detailed “Structured Logging” section) look accurate and helpful. However, per the prior review and the current summary, this
## Structured Loggingblock appears twice inAGENTS.md(one existing, one in this hunk).To avoid drift and maintenance headaches, keep a single canonical “Structured Logging” section and remove/merge the duplicate so all future edits only need to touch one place.
Also applies to: 53-53, 97-131
🧹 Nitpick comments (6)
lib/db/repos/providers.ts (1)
5-9: Include minimal error details in batch insert warningUsing a contextual logger here is good, and keeping the batch fallback logic unchanged is safe. You might want to capture at least a coarse error signal (e.g., message or code) in the log context so diagnosing systematic batch failures is easier:
} catch (err) { logger.warn("batch insert partial failure", { count: toCreate.length, error: err instanceof Error ? err.message : String(err), }); // ... }Also applies to: 298-299
lib/db/pglite.ts (1)
4-12: Console wrapping and logging look good; consider log level for benign close failuresThe console log suppression and restoration around
apply()are carefully scoped and safe, and the new logger wiring is consistent with the rest of the PR.Given the comment explicitly treats close errors as non-fatal (“may already be closed”) you might consider using
logger.warninstead oflogger.errorinclosePGliteDbto better reflect severity:} catch (err) { // Swallow errors on close (client may already be closed) logger.warn("close warning", { error: err }); }Also applies to: 33-44, 55-62
lib/db/repos/domains.ts (1)
7-10: Align log level with documented “warning” semanticsStructured logging here (including the domain in context) is a nice improvement. The JSDoc says this path “logs warnings without throwing”, but the implementation now uses
logger.error. If these failures are expected to be non-fatal noise, consider downgrading tologger.warn:} catch (err) { logger.warn("failed to update lastAccessedAt", { error: err, domain: name }); }Also applies to: 95-115
lib/cloudflare.ts (1)
7-10: Good use of structured logs around Cloudflare IP range fetchLogging both the successful fetch and the error path via the shared logger is correct and keeps behavior (empty ranges on failure) unchanged. If you want richer diagnostics later, you could add context like status or URL:
logger.info("IP ranges fetched", { source: "cloudflare-api" }); logger.error("fetch error", err, { url: CLOUDFLARE_IPS_URL, });But this is optional; current code is fine.
Also applies to: 90-99
server/services/seo.ts (1)
15-16: Consider logging the scheduling error details for better observabilityThe new logger usage (startup debug, final info summary, and warn on schedule failure) is consistent and helpful. One small gap: in the
after(() => scheduleRevalidation(...)).catch((_err) => { ... })block,_erris currently discarded and only the domain is logged.To make schedule failures easier to debug, consider including the error, e.g.:
after(() => { return scheduleRevalidation(/* ... */); }).catch((err) => { // either upgrade to error with stack: logger.error(`schedule failed for ${registrable}`, err, { domain: registrable }); // or at least capture a string version in context while keeping warn level: // logger.warn(`schedule failed for ${registrable}`, { // domain: registrable, // error: String(err), // }); });Not blocking, but would materially improve diagnostics when scheduling fails.
Also applies to: 28-29, 35-35, 273-283, 287-293
hooks/use-logger.ts (1)
1-50: useLogger hook is well-structured and memoization approach is appropriateThe hook cleanly wraps the client logger:
- Uses
JSON.stringify(baseContext)as a stable memo key for typical simple context objects, with a safe fallback to the object ref on serialization failure.- Returns a per-context
createLogger(baseContext)or the sharedclientLoggerwhen no context is provided.- Dependency management (
[contextKey]plus the explicit lint suppression) matches the intended behavior of “same logical context ⇒ same logger instance”.As long as
baseContextstays small and simple (as in the examples), this is a solid pattern with no correctness concerns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
AGENTS.md(3 hunks)components/domain/section-error-boundary.tsx(3 hunks)hooks/use-domain-export.ts(4 hunks)hooks/use-logger.ts(1 hunks)instrumentation.ts(1 hunks)lib/blob.ts(2 hunks)lib/cloudflare.ts(2 hunks)lib/db/pglite.ts(3 hunks)lib/db/repos/domains.ts(2 hunks)lib/db/repos/providers.ts(2 hunks)lib/edge-config.ts(2 hunks)lib/fetch-remote-asset.ts(8 hunks)lib/fetch.ts(2 hunks)lib/logger/correlation.ts(1 hunks)lib/logger/index.test.ts(1 hunks)lib/logger/index.ts(1 hunks)lib/logger/server.ts(1 hunks)lib/puppeteer.ts(4 hunks)lib/rdap-bootstrap.ts(2 hunks)lib/schedule.ts(4 hunks)lib/storage.ts(4 hunks)server/services/certificates.ts(3 hunks)server/services/favicon.ts(6 hunks)server/services/ip.ts(5 hunks)server/services/pricing.ts(6 hunks)server/services/screenshot.ts(8 hunks)server/services/seo.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/logger/correlation.ts
🧰 Additional context used
🧬 Code graph analysis (8)
server/services/seo.ts (1)
lib/schemas/domain/seo.ts (1)
SeoResponse(94-94)
server/services/certificates.ts (1)
lib/schemas/domain/certificates.ts (1)
Certificate(15-15)
lib/logger/index.test.ts (1)
lib/logger/index.ts (6)
shouldLog(100-105)getMinLogLevel(87-95)parseLogLevel(65-79)serializeError(110-125)createLogEntry(137-181)formatLogEntry(130-132)
hooks/use-logger.ts (3)
lib/logger/index.ts (2)
LogContext(14-14)Logger(35-42)lib/logger/server.ts (1)
createLogger(262-277)lib/logger/client.ts (1)
createLogger(190-205)
instrumentation.ts (1)
lib/logger/server.ts (2)
logger(250-250)error(225-227)
lib/logger/server.ts (1)
lib/logger/index.ts (7)
Logger(35-42)LogLevel(12-12)parseLogLevel(65-79)LogContext(14-14)shouldLog(100-105)createLogEntry(137-181)formatLogEntry(130-132)
hooks/use-domain-export.ts (3)
hooks/use-logger.ts (1)
useLogger(30-50)lib/analytics/server.ts (1)
analytics(66-138)lib/analytics/client.ts (1)
analytics(26-29)
lib/logger/index.ts (2)
lib/logger/server.ts (1)
error(225-227)lib/logger/client.ts (1)
error(153-155)
🔇 Additional comments (23)
lib/logger/server.ts (1)
30-55: Correlation ID and OpenTelemetry context handling look solidAsyncLocalStorage-based correlation ID propagation plus defensive
getTraceContext()(guarded by try/catch) give you enriched logs without risking runtime failures when tracing is unavailable. No issues from a correctness standpoint.Also applies to: 61-78
lib/rdap-bootstrap.ts (1)
6-9: RDAP bootstrap logging integration looks goodThe module-scoped logger with
source: "rdap-bootstrap"and the post-fetchlogger.info("bootstrap data fetched")are straightforward and won’t spam logs thanks to caching. No behavior changes beyond observability.Also applies to: 35-37
lib/blob.ts (1)
4-7: Robust error logging for blob deletionsSwitching to
logger.errorhere, wrapping unknowns asErrorand including theurlin context, gives you structured, PostHog-friendly error events without changing the per-URL result behavior. This looks solid.Also applies to: 50-56
components/domain/section-error-boundary.tsx (1)
9-10: Client logger integration in SectionErrorBoundary is appropriateCreating a component-scoped logger and logging render errors with both the thrown
errorand structured context (section,componentStack) is consistent with the new logging API and augments the existing PostHog capture without changing UI behavior.Also applies to: 26-27, 44-47
lib/fetch.ts (1)
1-4: LGTM! Clean logger integration.The structured logger is properly initialized with a descriptive source identifier, and the cleanup error handling correctly logs without propagating errors (ensuring all cleanup functions run).
server/services/favicon.ts (2)
8-12: LGTM! Structured logging properly integrated.Logger initialization is clean and the source identifier clearly identifies the module.
43-46: LGTM! Consistent logging with rich context.All console calls have been replaced with appropriate structured logger calls. The logging levels are well-chosen (debug for cache operations, warn for cleanup issues, error for persistence failures), and contextual metadata (domain, timeoutMs, cached status) enhances observability.
Also applies to: 63-67, 94-96, 99-100, 163-164, 201-202
lib/storage.ts (1)
5-9: LGTM! Upload retry flow logging is well-structured.The logger integration properly captures the upload lifecycle with appropriate levels (debug for attempts/delays, info for success, warn for failures) and includes useful structured metadata (pathname, attempts, durationMs) for debugging upload issues.
Also applies to: 61-76, 81-97
lib/edge-config.ts (1)
2-4: LGTM! Clean logging migration with graceful error handling.The structured logger is properly integrated, and the error handling maintains the existing graceful degradation behavior while providing better observability.
Also applies to: 40-43
server/services/certificates.ts (1)
13-20: LGTM! Comprehensive logging coverage for certificate operations.The structured logger is properly integrated throughout the certificate fetching flow with appropriate levels (debug for start, info for cache hits and success, warn for schedule failures, error for TLS errors). The contextual metadata (domain, count, chainLength, cached status) provides good observability.
Also applies to: 22-22, 70-76, 184-196, 198-201
lib/fetch-remote-asset.ts (1)
4-6: LGTM! Security-focused logging properly structured.All SSRF protection points now use structured logging with consistent warn level and rich context (url, reason). This provides excellent visibility for security monitoring and debugging of blocked requests.
Also applies to: 118-122, 133-137, 189-194, 200-208, 212-220, 228-236, 239-244, 247-255
server/services/pricing.ts (1)
3-7: LGTM! Provider integration logging is comprehensive.Structured logging properly captures both success and error paths for pricing providers with appropriate levels (info for success, error for failures/timeouts). The contextual metadata (provider name, status codes) enables effective filtering and debugging of upstream API issues.
Also applies to: 56-64, 95-112, 144-186
hooks/use-domain-export.ts (1)
4-4: LGTM! Client-side logging properly integrated with React patterns.The
useLoggerhook is correctly used for component-scoped logging, andlogger.erroris appropriately added to the dependency array. The error handling maintains graceful degradation while providing structured logging with domain context. The consistent use of theerrvariable throughout the catch block improves code clarity.Also applies to: 23-23, 86-105
lib/puppeteer.ts (1)
3-5: Structured logging integration for Puppeteer looks solidLogger creation and all three call sites (browser creation failure, close failure, and shutdown handler) correctly use the server logger API and preserve existing control flow. No issues from a correctness or behavior standpoint.
Also applies to: 121-121, 136-136, 145-145
server/services/ip.ts (1)
2-5: Logging around IP lookup is correctly wired and informativeThe new
createLogger({ source: "ip" })usage and the debug/info/error calls are correctly shaped:
- Start debug log provides a simple “start lookup” marker.
- Non-OK responses and thrown errors log via
logger.errorwith appropriate context (type,status,statusMessage).- Success path logs owner/domain at
infolevel.The
logger.error(..., undefined, { ... })pattern in the non-error-object branch correctly preserves context without inventing a fakeError. Behavior is unchanged aside from richer logging.Also applies to: 27-27, 39-45, 101-105, 114-114
server/services/screenshot.ts (1)
10-16: Screenshot service logging is consistent and non-invasiveThe added logger and call sites provide useful observability without altering behavior:
- In-flight request and DB cache-hit debug logs help reason about de-duplication and caching.
- Stale promise cleanup and high
screenshotPromisessize are correctly logged atwarnwith relevant context (domain,timeoutMs,count).- DB read/persist failures and
page.closefailures are logged aterrorwith the domain attached.No correctness or flow changes; this is a straightforward, well-scoped instrumentation upgrade.
Also applies to: 66-67, 88-92, 100-103, 138-145, 228-229, 248-251, 279-280
lib/schedule.ts (1)
12-15: Revalidation scheduling logs are clear and correctly leveledThe new
schedulelogger and its call sites give a good signal:
- Skips due to inactivity are logged at
infowith domain/section/lastAccessedAt.- Decay application is logged at
infowithdecayMultiplieranddaysInactive.- Successful Inngest scheduling is logged at
debug, and failures aterrorwith domain/section context.Behavior is unchanged, and these logs should make revalidation behavior significantly easier to diagnose.
Also applies to: 66-71, 88-93, 123-127, 130-133
lib/logger/index.test.ts (1)
1-115: Logger core tests accurately exercise the public helpersThis suite cleanly covers the core helpers:
- Level parsing/filtering (
parseLogLevel,shouldLog,getMinLogLevel) behaves as expected in a test environment.serializeErrorhandles both Error and non-Error inputs.createLogEntrytests validate inclusion of context, serialized error, correlation/trace/span IDs, and environment.formatLogEntryis verified to produce JSON with the expected fields.Everything is aligned with the implementations in
lib/logger/index.ts; no adjustments needed.instrumentation.ts (1)
14-25: LGTM! Clean migration to structured logging.The refactoring correctly replaces analytics error tracking with the new structured logger. The dynamic import is appropriate for instrumentation hooks, and the error handling maintains graceful degradation without throwing on logging failures.
lib/logger/index.ts (4)
1-42: Well-structured type definitions for the logging system.The types provide a comprehensive foundation for the logging subsystem.
LogEntryincludes all necessary fields for structured logging with OpenTelemetry integration (traceId, spanId) and correlation support. TheLoggerinterface establishes a consistent API across server and client environments.
48-105: Log level utilities are correctly implemented.The priority-based level system with environment-aware defaults (test→warn, development→debug, production→info) provides sensible behavior. The
shouldLogfiltering logic correctly compares numeric priorities.
110-125: Error serialization handles both Error and unknown types.The function correctly extracts standard Error properties (name, message, stack, cause) and provides a safe fallback for non-Error types using
String(error).
137-181: No changes required—environment handling is correct.The client logger properly calls
createLogEntry(lines 53, 97 inlib/logger/client.ts) and directly accessesprocess.env.NODE_ENV(lines 35, 62), confirming the bundler provides this variable in the client context. The sharedcreateLogEntryfunction's access toprocess.env.NODE_ENVat line 176 is therefore consistent with how the client logger operates. The code correctly enriches log entries with environment information across both client and server implementations.
….error across multiple services
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/error.tsx (1)
16-16: Logger properly serializes Error objects—no changes needed. Consider digest as optional tracing enhancement.The logger correctly serializes Error objects via
serializeError(), which extractsmessage,stack,name, andcause. The initial concern about Error objects serializing to{}is unfounded.However, passing
error.digestas explicit context metadata is a valid suggestion for improved traceability, since the digest is already displayed in the UI and valuable for error tracking:- logger.error("Root error boundary caught error", error); + logger.error("Root error boundary caught error", error, { + digest: error.digest, + });lib/analytics/client.ts (1)
14-16: AligntrackExceptiondoc with its public exposure and examplesThe new JSDoc says
@internaland recommends usinglogger.error()instead, buttrackExceptionis still exported viaanalyticsand surfaced byuseAnalytics, and the hook’s example continues to showtrackExceptionusage. That’s a mixed signal to contributors about the intended API.Consider either:
- If
trackExceptionshould be truly internal: stop exporting it fromanalyticsanduseAnalytics, and update the example to rely onlogger.error()instead; or- If you still want it available but discouraged: soften the wording to “Prefer
logger.error()in application code; this exists as a low‑level exception hook” and also adjust the example to match that guidance.For the comment itself, something like this would better reflect the intent without contradicting the current exports:
-/** - * @internal Use logger.error() instead, which automatically tracks exceptions. - */ +/** + * Prefer logger.error() in application code; this exists as a low-level exception hook. + */hooks/use-domain-export.ts (1)
86-99: TightenuseCallbackdeps: depend onloggerinstead oflogger.errorThe error handling and toast description are solid, but in the dependency array you typically want to depend on the logger object, not a specific method property. This is slightly clearer and avoids any surprises if the logger implementation ever changes method bindings.
- }, [domain, queryClient, queryKeys, logger.error]); + }, [domain, queryClient, queryKeys, logger]);Please confirm with your React linting setup (e.g.,
react-hooks/exhaustive-deps) that switching the dependency fromlogger.errortologgerkeeps the rule satisfied and doesn’t introduce new warnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/error.tsx(2 hunks)app/global-error.tsx(2 hunks)components/domain/create-section-with-data.test.tsx(1 hunks)hooks/use-domain-export.ts(3 hunks)lib/analytics/client.ts(1 hunks)lib/analytics/server.ts(1 hunks)lib/db/repos/providers.ts(2 hunks)lib/fetch.ts(3 hunks)server/services/certificates.ts(3 hunks)server/services/dns.ts(7 hunks)server/services/headers.ts(5 hunks)server/services/hosting.ts(6 hunks)server/services/registration.ts(6 hunks)server/services/seo.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- components/domain/create-section-with-data.test.tsx
- lib/analytics/server.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/services/registration.ts
- server/services/hosting.ts
- server/services/dns.ts
- server/services/headers.ts
- server/services/seo.ts
🧰 Additional context used
🧬 Code graph analysis (2)
server/services/certificates.ts (1)
lib/schemas/domain/certificates.ts (1)
Certificate(15-15)
hooks/use-domain-export.ts (1)
hooks/use-logger.ts (1)
useLogger(30-50)
🔇 Additional comments (16)
lib/fetch.ts (3)
1-3: LGTM: Clean structured logging initialization.The logger initialization with
source: "fetch"provides clear provenance for log entries from this utility module.
33-39: Excellent structured logging for retry attempts.The structured context (url, error, attempt details) significantly improves observability and debugging capability compared to the previous approach. The
warnlevel is appropriate for transient failures.
165-165: Appropriate error logging in cleanup handler.The use of
logger.errorhere correctly logs cleanup failures while allowing remaining cleanup functions to execute. The error is properly contained within the cleanup loop.lib/db/repos/providers.ts (3)
5-8: LGTM: Clean logger setup.The logger import and instantiation follow the established pattern for the centralized logging refactor, with an appropriate source label.
13-13: LGTM: Type definition improvement.Adding the optional
namefield toResolveProviderInputimproves type safety and accurately reflects the actual usage throughout the code.
299-301: The logger.error API signature matches the usage correctly.Object literal types have implicit index signatures for assignability, so the metadata object
{ count: toCreate.length }is properly compatible with the LogContext parameter. The structured logging with context metadata is a good improvement over console.warn and the error handling pattern (log then fall back) is appropriate.server/services/certificates.ts (6)
13-13: LGTM: Clean logger setup.The import and logger instantiation follow the structured logging pattern. Scoping the logger to
"certificates"provides useful context for filtering and querying logs.Also applies to: 19-19
22-22: LGTM: Appropriate entry point logging.Debug-level logging at the function entry is appropriate. Logging the raw input
domain(before validation) is useful for tracing requests with invalid inputs.
70-74: LGTM: Well-structured cache hit logging.The info-level logging with rich context (
domain,count,cached) is excellent for observability and enables useful metrics on cache effectiveness.
192-195: LGTM: Clear success logging with operational context.The info-level logging captures successful certificate retrieval with useful operational data (
chainLength), which aids in monitoring and debugging.
198-198: LGTM: Appropriate error logging with graceful degradation.Error logging for TLS connection failures is appropriate. The existing graceful degradation (returning empty array) ensures the service remains resilient while capturing errors for observability.
185-187: Error logging at lines 185-187 is correctly implemented.The logger API signature
error(message: string, error?: Error | unknown, context?: LogContext)is properly used. The caught error object is correctly passed as the second parameter and will include stack traces, with the domain context provided for correlation. This usage is consistent throughout the codebase.app/error.tsx (1)
7-7: LGTM: Client logger import is appropriate.The migration from PostHog to the centralized client logger is correct for this client component.
app/global-error.tsx (2)
5-5: LGTM: Consistent logger import.The client logger import is appropriate and consistent with the error boundary migration pattern.
15-15: Verify Error object serialization (same as error.tsx).The same Error serialization concern applies here. Ensure the logger properly extracts Error properties (
message,stack,name) for structured logging. As the global error boundary is the last resort for error handling, proper observability here is critical.Consider adding explicit metadata:
- logger.error("Global error boundary caught error", error); + logger.error("Global error boundary caught error", error, { + digest: error.digest, + });The verification script from the error.tsx review applies here as well.
hooks/use-domain-export.ts (1)
4-24: Contextual logger integration forDomainExportlooks goodUsing
useLogger({ component: "DomainExport" })here is a good fit with the new structured logging system and should produce stable, scoped logs for this hook given the memoization behavior inuseLogger.Please double‑check that the logger context shape
{ component: "DomainExport" }matches how you intend to filter/aggregate logs elsewhere (dashboards, log queries), and adjust field names if you have any emerging conventions.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.