feat: Add sindarian-logs unified logging package with middleware support#83
feat: Add sindarian-logs unified logging package with middleware support#83caioaletroca wants to merge 2 commits intodevelopfrom
Conversation
WalkthroughA new logging and tracing package Sequence DiagramsequenceDiagram
participant Client
participant TraceMiddleware
participant LoggerAggregator
participant RequestIdRepository
participant LoggerRepository
participant Handler
Client->>TraceMiddleware: HTTP Request
activate TraceMiddleware
TraceMiddleware->>RequestIdRepository: generate() / runWith(traceId)
activate RequestIdRepository
RequestIdRepository->>LoggerAggregator: runWithContext(path, method, {traceId})
activate LoggerAggregator
LoggerAggregator->>Handler: next()
activate Handler
Handler->>LoggerAggregator: addEvent(operation, level, message)
LoggerAggregator->>LoggerAggregator: buffer event
Handler-->>LoggerAggregator: Response
deactivate Handler
LoggerAggregator->>LoggerAggregator: setResponseMetadata(statusCode)
LoggerAggregator->>LoggerAggregator: finalizeContext()
LoggerAggregator->>LoggerRepository: info/error/warn/debug/audit(AggregatedLog)
activate LoggerRepository
LoggerRepository->>LoggerRepository: serialize & write via Pino
deactivate LoggerRepository
LoggerAggregator-->>TraceMiddleware: Promise<Response>
deactivate LoggerAggregator
deactivate RequestIdRepository
TraceMiddleware-->>Client: Response
deactivate TraceMiddleware
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sindarian-logs/eslint.config.mjs`:
- Line 21: The ESLint config uses import.meta.dirname in eslint.config.mjs which
requires Node >=18.17.0; update the package.json to declare the minimum Node
engine (e.g. add "engines": { "node": ">=18.17.0" }) so environments with older
Node don't fail loading the config, and ensure any CI/tooling respects the
engines field or document the requirement in README if you prefer a different
minimum.
In `@packages/sindarian-logs/package.json`:
- Around line 22-24: The package's "exports" only points the "." entry to
"./dist/index.js" so TypeScript can't find types for moduleResolution modes;
update the "exports" "." entry to include a types mapping pointing to
"./dist/index.d.ts" (or use conditional exports with "import"/"require" and a
"types" or "types" condition) alongside the existing "./dist/index.js" so
consumers using bundler/node16 resolution can locate type definitions for the
package.
In `@packages/sindarian-logs/src/aggregator/logger-aggregator.ts`:
- Around line 212-223: The initializer for highest in escalateLevel currently
forces a minimum of 'info' so debug-only aggregates can never win; change
escalateLevel to initialize highest from the events (e.g., const highest =
events[0]?.level ?? 'debug') or to the lowest-priority level ('debug') before
iterating so a list of debug events can produce an aggregate 'debug'; apply the
same fix to the other aggregator method that uses the same "highest = 'info'"
pattern so downstream writeLog() can see true debug-only aggregates.
- Around line 47-60: The current finalizeContext() and logging calls
(addEvent/finalizeContext inside the storage.run wrapper around fn()) can throw
and replace the original outcome; make finalization best-effort by wrapping
addEvent() and finalizeContext() calls in their own try/catch blocks so any
errors from LoggerRepository/serialization are caught and swallowed (or logged
to a lower-level logger) and do not propagate; ensure in the catch branch you
rethrow the original caught error after attempting (but not allowing)
finalization, and in the success branch return the original result even if
finalizeContext() fails; update the storage.run wrapper around fn(), referencing
the existing fn, addEvent, and finalizeContext symbols to implement this
behavior.
In `@packages/sindarian-logs/src/decorators/http-log-helper.ts`:
- Around line 39-47: In the catch handler (where methodName === 'catch') avoid
blind casts to Request/Response: add instanceof checks like in
onBeforeFetch/onAfterFetch to verify args[0] is a Request and args[1] is a
Response before accessing .method, .url, or .status (in the catch branch in
http-log-helper.ts), and fallback to a safe stringification or placeholder when
types don’t match so logger.error(operation, ...) never reads undefined
properties or throws.
In `@packages/sindarian-logs/src/decorators/traceable.ts`:
- Around line 47-72: The root-context branch created by logger.runWithContext
currently doesn't emit the same "Entering {operation}" and "Completed
{operation}" logs or mirror the nested branch's error handling; update the async
callback passed to logger.runWithContext (the function that calls
originalMethod) to call logger.info(operation, `Entering ${operation}`) before
invoking originalMethod, log logger.info(operation, `Completed ${operation}`)
after a successful result, and catch errors to call logger.error(operation,
error instanceof Error ? error.message : String(error)) before
rethrowing—matching the behavior of the logger.hasContext branch and using the
same operation/originalMethod symbols.
- Around line 32-36: The wrapper assigned to descriptor.value is declared async,
forcing all decorated methods to return Promise<T>; change descriptor.value to a
non-async function that calls originalMethod.apply(this, args), captures its
return as const result, and then handle both sync and async results (e.g., if
result is a Promise or via Promise.resolve(result) for unified post-processing)
so synchronous methods keep their original return types while async methods are
awaited; update traceable decorator logic that references descriptor.value and
originalMethod accordingly and add a short comment documenting that both sync
and async returns are supported.
In `@packages/sindarian-logs/src/logger-module.ts`:
- Around line 12-26: The current provider eagerly constructs a
PinoLoggerRepository with debug resolved at module load, while LoggerAggregator
reads ENABLE_DEBUG inside its useFactory at resolution time, which can cause
inconsistent debug settings; change the LoggerRepository provider to use a
factory (useFactory) that instantiates and returns new PinoLoggerRepository({
debug: process.env.ENABLE_DEBUG === 'true' }) at resolution time so both
LoggerRepository and LoggerAggregator derive the debug flag consistently from
the environment when resolved (refer to LoggerRepository, PinoLoggerRepository,
LoggerAggregator and the useFactory pattern).
In `@packages/sindarian-logs/src/middleware/trace-middleware.test.ts`:
- Around line 82-92: Rename the existing test "should handle zero
content-length" to "should handle missing content-length header" since it uses
empty headers; update the it(...) description and keep the same body that calls
middleware.use and asserts mockLogger.setResponseMetadata was called with (204,
undefined). Then add a new test that constructs the response with headers: {
'content-length': '0' }, invokes middleware.use the same way, and asserts
mockLogger.setResponseMetadata was called with (204, 0) to verify actual zero
content-length handling; reference the middleware.use call and
mockLogger.setResponseMetadata in both tests.
- Around line 50-80: The test sets a mock implementation on
mockLogger.runWithContext that captures realRepo.get() outside the storage
context, which is unnecessary and returns undefined; remove the
mockLogger.runWithContext.mockImplementation(...) block (the override that
assigns capturedTraceId = realRepo.get()) so the test relies on the actual
runWithContext behavior and only captures capturedTraceId inside the mw.use /
next() callback; references: mockLogger.runWithContext, RequestIdRepository,
TraceMiddleware, mw.use, capturedTraceId.
In `@packages/sindarian-logs/src/middleware/trace-middleware.ts`:
- Around line 34-37: The current conversion of the Content-Length header uses
Number(response.headers.get('content-length')) || undefined which turns a real 0
into undefined; change the logic in the trace-middleware where
response.headers.get('content-length') is passed to
this.logger.setResponseMetadata so that you use a nullish check (e.g., use
nullish coalescing or an explicit isNaN check) to preserve numeric zero but
still treat missing or non-numeric values as undefined—i.e., compute length =
Number(response.headers.get('content-length')) and pass length if it is not NaN
(or use length ?? undefined) into this.logger.setResponseMetadata.
In `@packages/sindarian-logs/src/repositories/pino-logger-repository.ts`:
- Around line 46-64: The repository currently stringifies logs before sending
them to Pino (in methods info, error, warn, debug, audit of the
pino-logger-repository), which collapses structured fields; change each method
to pass the AggregatedLog object directly to this.logger (e.g.,
this.logger.info(log), this.logger.error(log), etc.) so fields are emitted as
top-level JSON properties, and update pino-logger-repository.test.ts
expectations to assert the logged object shape instead of a JSON string.
In `@packages/sindarian-logs/src/types/log-event.ts`:
- Around line 12-20: AggregatedLog is missing a field to carry responseSize so
the value set by LoggerAggregator.setResponseMetadata (and present on
RequestContext.responseSize) never reaches downstream storage; add a nullable
numeric property named responseSize to the AggregatedLog type (e.g.,
responseSize?: number) and ensure LoggerAggregator (method setResponseMetadata)
assigns context.responseSize into the AggregatedLog object so LoggerRepository
receives it.
In `@packages/sindarian-logs/tsconfig.json`:
- Line 18: Update the main tsconfig.json types array to remove "jest" and add
"node" so build-time type checking uses Node lib types (necessary for
async_hooks/crypto) while leaving Jest types only in test/ESLint configs;
specifically edit the "types": ["jest", "reflect-metadata"] entry to replace
"jest" with "node" (tsconfig.json).
In `@packages/sindarian-server/src/services/middleware.ts`:
- Line 1: The APP_MIDDLEWARE token currently uses Symbol('APP_MIDDLEWARE') which
is consistent with the other DI tokens (APP_PIPE, APP_GUARD, APP_FILTER,
APP_INTERCEPTOR, REQUEST); keep it as Symbol() to maintain in-package
consistency; if you intend to require cross-package symbol identity instead,
change all DI tokens (APP_MIDDLEWARE, APP_PIPE, APP_GUARD, APP_FILTER,
APP_INTERCEPTOR, REQUEST) to use Symbol.for('...') in a single sweep rather than
changing only APP_MIDDLEWARE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba4c52e4-a2bf-4aa6-b00b-8b1101d9f247
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/sindarian-server/test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (46)
.github/workflows/release.ymlpackages/sindarian-logs/eslint.config.mjspackages/sindarian-logs/jest.config.tspackages/sindarian-logs/package.jsonpackages/sindarian-logs/setupJest.tspackages/sindarian-logs/src/aggregator/index.tspackages/sindarian-logs/src/aggregator/logger-aggregator.test.tspackages/sindarian-logs/src/aggregator/logger-aggregator.tspackages/sindarian-logs/src/decorators/http-log-helper.test.tspackages/sindarian-logs/src/decorators/http-log-helper.tspackages/sindarian-logs/src/decorators/index.tspackages/sindarian-logs/src/decorators/log-http-call.tspackages/sindarian-logs/src/decorators/traceable.tspackages/sindarian-logs/src/functions/index.tspackages/sindarian-logs/src/functions/with-trace.test.tspackages/sindarian-logs/src/functions/with-trace.tspackages/sindarian-logs/src/index.tspackages/sindarian-logs/src/logger-module.tspackages/sindarian-logs/src/middleware/index.tspackages/sindarian-logs/src/middleware/trace-middleware.test.tspackages/sindarian-logs/src/middleware/trace-middleware.tspackages/sindarian-logs/src/repositories/index.tspackages/sindarian-logs/src/repositories/logger-repository.tspackages/sindarian-logs/src/repositories/pino-logger-repository.test.tspackages/sindarian-logs/src/repositories/pino-logger-repository.tspackages/sindarian-logs/src/request-id/index.tspackages/sindarian-logs/src/request-id/request-id-repository.test.tspackages/sindarian-logs/src/request-id/request-id-repository.tspackages/sindarian-logs/src/services/index.tspackages/sindarian-logs/src/services/loggable-http-service.tspackages/sindarian-logs/src/types/index.tspackages/sindarian-logs/src/types/log-event.tspackages/sindarian-logs/tsconfig.eslint.jsonpackages/sindarian-logs/tsconfig.jsonpackages/sindarian-server/package.jsonpackages/sindarian-server/src/constants/keys.tspackages/sindarian-server/src/dependency-injection/injectable-decorator.tspackages/sindarian-server/src/index.tspackages/sindarian-server/src/middleware/index.tspackages/sindarian-server/src/middleware/middleware-handler.tspackages/sindarian-server/src/middleware/middleware.tspackages/sindarian-server/src/modules/module-decorator.tspackages/sindarian-server/src/server/server-factory.tspackages/sindarian-server/src/services/middleware.tspackages/sindarian-server/test/app/logger/logger-interceptor.tspackages/sindarian-server/test/app/logger/logger-module.ts
💤 Files with no reviewable changes (2)
- packages/sindarian-server/package.json
- packages/sindarian-server/test/app/logger/logger-module.ts
| this.logger.setResponseMetadata( | ||
| response.status, | ||
| Number(response.headers.get('content-length')) || undefined | ||
| ) |
There was a problem hiding this comment.
Actual content-length: 0 is incorrectly converted to undefined.
When the response has content-length: 0 (e.g., a 204 No Content or an empty body), Number("0") returns 0, and 0 || undefined evaluates to undefined, losing the actual zero value.
🛠️ Proposed fix using nullish coalescing
this.logger.setResponseMetadata(
response.status,
- Number(response.headers.get('content-length')) || undefined
+ response.headers.get('content-length') != null
+ ? Number(response.headers.get('content-length'))
+ : undefined
)Alternatively, if you want to treat non-numeric headers as undefined:
this.logger.setResponseMetadata(
response.status,
- Number(response.headers.get('content-length')) || undefined
+ (() => {
+ const cl = response.headers.get('content-length')
+ if (cl == null) return undefined
+ const num = Number(cl)
+ return Number.isNaN(num) ? undefined : num
+ })()
)📝 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.
| this.logger.setResponseMetadata( | |
| response.status, | |
| Number(response.headers.get('content-length')) || undefined | |
| ) | |
| this.logger.setResponseMetadata( | |
| response.status, | |
| response.headers.get('content-length') != null | |
| ? Number(response.headers.get('content-length')) | |
| : undefined | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sindarian-logs/src/middleware/trace-middleware.ts` around lines 34 -
37, The current conversion of the Content-Length header uses
Number(response.headers.get('content-length')) || undefined which turns a real 0
into undefined; change the logic in the trace-middleware where
response.headers.get('content-length') is passed to
this.logger.setResponseMetadata so that you use a nullish check (e.g., use
nullish coalescing or an explicit isNaN check) to preserve numeric zero but
still treat missing or non-numeric values as undefined—i.e., compute length =
Number(response.headers.get('content-length')) and pass length if it is not NaN
(or use length ?? undefined) into this.logger.setResponseMetadata.
| export type AggregatedLog = { | ||
| level: LogLevel | ||
| method: string | ||
| path: string | ||
| statusCode?: number | ||
| duration: number | ||
| traceId?: string | ||
| handler?: string | ||
| events: TransformedEvent[] |
There was a problem hiding this comment.
Expose responseSize on the aggregated log contract.
RequestContext already carries responseSize, and LoggerAggregator.setResponseMetadata() records it, but AggregatedLog has nowhere to store it. Right now that second parameter is effectively dead and never reaches LoggerRepository.
Proposed fix
export type AggregatedLog = {
level: LogLevel
method: string
path: string
statusCode?: number
+ responseSize?: number
duration: number
traceId?: string
handler?: string
events: TransformedEvent[]
}// packages/sindarian-logs/src/aggregator/logger-aggregator.ts
if (context.responseSize !== undefined) log.responseSize = context.responseSizeAlso applies to: 32-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sindarian-logs/src/types/log-event.ts` around lines 12 - 20,
AggregatedLog is missing a field to carry responseSize so the value set by
LoggerAggregator.setResponseMetadata (and present on
RequestContext.responseSize) never reaches downstream storage; add a nullable
numeric property named responseSize to the AggregatedLog type (e.g.,
responseSize?: number) and ensure LoggerAggregator (method setResponseMetadata)
assigns context.responseSize into the AggregatedLog object so LoggerRepository
receives it.
| @@ -0,0 +1 @@ | |||
| export const APP_MIDDLEWARE = Symbol('APP_MIDDLEWARE') | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== package manifests referencing `@lerianstudio/sindarian-server` ==="
fd package.json packages | while read -r f; do
python - "$f" <<'PY'
import json,sys
p=sys.argv[1]
with open(p) as fh:
d=json.load(fh)
name=d.get("name","<unknown>")
keys=("dependencies","peerDependencies","devDependencies")
found=False
for k in keys:
v=d.get(k,{}).get("@lerianstudio/sindarian-server")
if v is not None:
if not found:
print(f"\n{p} ({name})")
found=True
print(f" {k}: {v}")
PY
done
if [ -f package-lock.json ]; then
echo -e "\n=== lockfile entries for `@lerianstudio/sindarian-server` ==="
rg -n '"@lerianstudio/sindarian-server"|node_modules/@lerianstudio/sindarian-server' package-lock.json -C1
fiRepository: LerianStudio/console-sdk
Length of output: 798
🏁 Script executed:
# Search for APP_MIDDLEWARE usage
rg -n "APP_MIDDLEWARE" --type ts --type js -B2 -A2Repository: LerianStudio/console-sdk
Length of output: 4427
🏁 Script executed:
# Check for Symbol usage in dependency injection context
rg -n "Symbol\(" packages/sindarian-server/src --type ts -B2 -A2Repository: LerianStudio/console-sdk
Length of output: 11851
🏁 Script executed:
# Check inversify usage and container setup
fd -e ts -e js packages/sindarian-server/src | xargs rg -l "Container|injectable|inject" | head -5Repository: LerianStudio/console-sdk
Length of output: 694
🏁 Script executed:
# Check all package versions and dependency ranges
rg -n "@lerianstudio/sindarian-server|inversify" packages/*/package.json --type json -B1 -A1Repository: LerianStudio/console-sdk
Length of output: 767
🏁 Script executed:
# Check if sindarian-logs or other packages could depend on multiple versions
cat packages/sindarian-logs/package.jsonRepository: LerianStudio/console-sdk
Length of output: 1226
🏁 Script executed:
# Look for inversify documentation or patterns in how symbols are used
rg -n "Symbol\.for\|getAllAsync" packages --type ts -B2 -A2 | head -40Repository: LerianStudio/console-sdk
Length of output: 50
🏁 Script executed:
# Check if there's any test or documentation about symbol identity with multiple copies
rg -n "symbol|Symbol" packages/sindarian-server/TECHNICAL.md -B2 -A2Repository: LerianStudio/console-sdk
Length of output: 401
🏁 Script executed:
# Verify all DI token exports to understand the pattern
cat packages/sindarian-server/src/index.ts | grep -A50 "export.*Symbol\|APP_"Repository: LerianStudio/console-sdk
Length of output: 441
🏁 Script executed:
# Check if sindarian-logs re-exports or has any symbol handling
rg -n "Symbol\|APP_MIDDLEWARE\|Symbol\.for" packages/sindarian-logs/src --type tsRepository: LerianStudio/console-sdk
Length of output: 50
🏁 Script executed:
# Check if there are any actual inversify errors or resolution failures in tests
rg -n "No bindings found\|resolution\|duplicate" packages/sindarian-server/src --type ts -B2 -A2 | head -30Repository: LerianStudio/console-sdk
Length of output: 50
🏁 Script executed:
# Check how other libraries in similar monorepo patterns handle this
rg -n "Symbol\.for" . --type ts --type json | head -20Repository: LerianStudio/console-sdk
Length of output: 50
🏁 Script executed:
# Verify if consuming sindarian-logs from sindarian-server could lead to duplicate copies
cat packages/sindarian-server/package.json | grep -A20 "dependencies\|peerDependencies"Repository: LerianStudio/console-sdk
Length of output: 310
Symbol.for() is a best practice but not required in this monorepo context.
The peer dependency structure with a single package-lock.json ensures @lerianstudio/sindarian-server resolves to one copy, preventing symbol identity divergence. Additionally, all similar DI tokens in the codebase (APP_PIPE, APP_GUARD, APP_FILTER, APP_INTERCEPTOR, REQUEST) consistently use Symbol() rather than Symbol.for(), so changing only APP_MIDDLEWARE would introduce inconsistency within the package itself.
Symbol.for() becomes critical when packages are duplicated across external npm dependencies. In this monorepo with peer dependencies, the current implementation is stable. However, if this package will be consumed by external projects expecting strict symbol identity across multiple copies, consider aligning all DI tokens to Symbol.for() consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sindarian-server/src/services/middleware.ts` at line 1, The
APP_MIDDLEWARE token currently uses Symbol('APP_MIDDLEWARE') which is consistent
with the other DI tokens (APP_PIPE, APP_GUARD, APP_FILTER, APP_INTERCEPTOR,
REQUEST); keep it as Symbol() to maintain in-package consistency; if you intend
to require cross-package symbol identity instead, change all DI tokens
(APP_MIDDLEWARE, APP_PIPE, APP_GUARD, APP_FILTER, APP_INTERCEPTOR, REQUEST) to
use Symbol.for('...') in a single sweep rather than changing only
APP_MIDDLEWARE.
Summary
sindarian-logspackage: Unified, request-scoped logging system built on AsyncLocalStorage and Pino,replacing the external
@lerianstudio/lib-logsdependencysindarian-server: AddedMiddlewareabstract class,MiddlewareHandler, andAPP_MIDDLEWAREtoken to wrap the entire request pipeline (guards → interceptors → handler → filters)@Injectable()now stores the original class name viaCLASS_NAME_KEY, withgetClassName()utility that survives minificationWhat's included
sindarian-server changes
Middlewareabstract class +MiddlewareHandler.execute()(recursivecreateNextpattern matchingInterceptorHandler)APP_MIDDLEWAREmulti-provider token registered inServerFactoryCLASS_NAME_KEYmetadata +getClassName()exported utility@lerianstudio/lib-logsdependencysindarian-logs (new package)
LoggerAggregator— AsyncLocalStorage-based event collection with level escalation, MAX_EVENTS cap (1000), andper-request finalization
TraceMiddleware— generates trace IDs viaRequestIdRepository(AsyncLocalStorage-isolated), wraps requests inlogging context
PinoLoggerRepository— Pino integration with JSON output for OTEL/Grafana, optional pino-pretty in dev@Traceable()— zero-config method decorator, auto-derivesClassName.methodName, context-aware (nested vs root)@LogHttpCall()— decorator for HTTP hooks when extendingLoggableHttpServiceisn't possibleLoggableHttpService— base class with automatic request/response loggingwithTrace()— wrapper for non-class code (NextAuth callbacks, cron jobs, queues)LoggerModule— ready-to-use DI module with all providers configuredCI/CD
sindarian-logsto workflow_dispatch release optionstest:e2escript placeholderTest plan