feat: add structuredErrorHandler middleware alongside errorMiddleware

[bheddens]

Summary

Adds a new structuredErrorHandler export to src/sdk/middleware.ts that pairs with the existing errorMiddleware. Consumers can opt-in for structured logging without touching their current errorMiddleware(...) wiring.

This is the response to review feedback on screener-integration-service#87 — lift the fix into the shared SDK so every integration benefits with one change.

Why

Today's errorMiddleware(onError) is generic: it sets a 500 response and invokes a side-effect callback with the error. Several Envoy integration services pass console.log as the callback, which stringifies via error.toString() and produces bare INFO log lines in Datadog with:

• no stack trace
• no operation tag
• no integration name / install id / company id context
• no severity (INFO instead of ERROR)

Errors like "Error: Could not initialize plugin client." end up unsearchable, undebuggable.

API

import { structuredErrorHandler, StructuredErrorLogger } from '@envoy/envoy-integrations-sdk';

// Default: read req.logger (if upstream middleware attached one), else console.error.
app.use(structuredErrorHandler());

// Override: take full control of how the error is logged.
app.use(structuredErrorHandler((err, req) => {
  myLogger.error('Plugin pipeline error', err, { reqId: (req as any).id });
}));

The exported StructuredErrorLogger interface (just error(message, error, metadata?)) lets consumers type their req.logger against the contract the SDK consumes.

Why a new export instead of changing errorMiddleware

Per PR review feedback: a new name means existing consumers that switch over without first wiring req.logger upstream get an observable console.error(err) fallback instead of silent data loss. They can adopt at their own pace.

Default behavior

req.logger state	Behavior
req.logger.error(...) is a function	Calls it with (message, err, { operation, httpMethod, httpUrl })
req.logger missing or malformed	console.error(err) so the raw error is at least surfaced
req.logger.error throws	Caught; logged as console.error('structuredErrorHandler: req.logger.error threw', logErr) so a buggy logger never masks the original error

The 500 / application/json / { message } response shape is identical to errorMiddleware.

Test plan

• tsc --noEmit clean
• eslint clean (test file uses /* eslint-disable no-console */ around the spy block, matching the pattern in test/util/userAgent.test.ts)
• All 80 SDK tests pass (72 existing + 8 new)
• Once published: bump SDK in screener-integration-service and replace the inline implementation from Fix deprecated actions/cache v2 in package workflow #87 with the SDK export.

New test coverage

• Logger-present happy path emits (message, err, metadata) with operation, httpMethod, httpUrl
• Falls back to console.error when req.logger is missing or lacks an error method
• Catches and surfaces exceptions thrown by req.logger.error without losing the original error
• onError override receives (err, req) and bypasses the default logger lookup
• Catches and surfaces exceptions thrown by the onError callback
• 500 / JSON response when headers haven't been sent
• Delegates to next(err) when headers have already been sent

Downstream

Once this merges and publishes, screener-integration-service#87 will be reworked to consume structuredErrorHandler from the SDK instead of duplicating the implementation locally. Same opportunity for pacs, presence, emno, and any other integration services that currently use errorMiddleware(console.log).

---

Architecture note — AsyncLocalStorage as a follow-up

This PR sticks with the established Envoy pattern of mutating req.logger across the middleware chain. That choice is deliberate — it matches screener-integration-service, pacs-integration-service, and every other Envoy service today, so there's no per-service migration cost beyond swapping errorMiddleware(console.log) → structuredErrorMiddleware() and wiring loggerFactory / envoyLoggerContextMiddleware.

However, the modern Node (16+) best practice for request-scoped context is AsyncLocalStorage from async_hooks, and it's worth flagging that as a follow-up rather than letting this PR enshrine the req.logger pattern indefinitely.

What AsyncLocalStorage would change

import { AsyncLocalStorage } from 'async_hooks';

const loggerStore = new AsyncLocalStorage<StructuredLogger>();

export function getRequestLogger(): StructuredLogger | undefined {
  return loggerStore.getStore();
}

// One middleware does the whole job:
export function envoyMiddleware(options: { logger: StructuredLogger }): RequestHandler {
  return (req, res, next) => {
    // ...body parse + envoy init as today...
    const requestLogger = options.logger.child({
      requestId: makeId(),
      ...extractLoggerContext(req as EnvoyRequest),
    });
    req.logger = requestLogger;
    loggerStore.run(requestLogger, () => next());  // ← key line
  };
}

Then anywhere in the call stack — services, helpers, vendor clients — getRequestLogger().error(...) returns the correct request's logger with no req plumbing.

Advantages

• No req plumbing. Services 4 layers deep into vendor clients can log with full context without each function signature growing a logger parameter. Today this is the single biggest source of context-less logs in our codebases.
• Collapses the API surface. loggerFactory + envoyLoggerContextMiddleware + addRequestLoggerContext + the if-present-child-else-construct dance all reduce to one middleware and one getRequestLogger() helper.
• Stronger isolation than a registry. Context binds to the async resource chain (Promise → Promise → timer → I/O callback), not to a Map<requestId, ctx>. There's no code path that could look up the wrong context for a concurrent request.
• Automatic cleanup. When a request's async chain unwinds, the stored value becomes unreachable and is GC'd. No delete map[id], no "request finished" hook, no per-request teardown to forget.
• Industry-standard. dd-trace (already in production here), OpenTelemetry, pino-http, NestJS's nestjs-cls, and Fastify's request.log all use ALS internally. It's well-trodden ground.

Disadvantages

• Modest runtime overhead. Node 16 added ~2–3% overhead per async op; Node 20 brought that down further with V8-level hooks. Not a concern at our scale, but non-zero.
• Leak vectors require discipline. Three real pitfalls — long-lived timers/intervals that outlive the request, module-level variables that capture the stored value, and unresolved Promises — all hold the context alive past request end. Same as any closure leak, but worth code-reviewing for.
• Harder to reason about in stack traces. "Where did this logger come from?" has no syntactic answer with ALS — you have to know the chain. req.logger is grep-able; getRequestLogger() requires understanding what als.run() wrapped the handler in.
• Test fixtures change. Today's tests stub req.logger. With ALS, tests have to wrap the unit under test in loggerStore.run(stubLogger, () => unitUnderTest()), or expose a test helper that does. Minor but pervasive.
• Migration cost is real. Every existing service that reads req.logger directly keeps working (we'd keep req.logger set as a compatibility surface), but code that wants to benefit from ALS — i.e. deep helpers that today take a logger param — has to change to call getRequestLogger(). This is an opt-in, per-call-site migration.

Recommendation

Land this PR as the structured-error fix for today, and open a separate RFC issue for the ALS migration as a follow-up. Combining both in one PR would balloon scope and risk.

If we do move to ALS later, the StructuredLogger interface and EnvoyLoggerContext types from this PR carry over unchanged — the change is purely how req.logger gets propagated to deep code, not what it looks like.

[JustWalters]

Don't forget to update package.json and package-lock.json

[JustWalters]

We're logging the error onError threw but the original error might be swallowed if onError throws before logging it. The comment suggests we'd log err.

[JustWalters]

What does this operation represent? I'd expect it to refer to something about the failed request. I could see 'structuredErrorHandler' being a useful value if operation is standard metadata we log, but I don't remember seeing it elsewhere.

[JustWalters]

Most of the places we're using the result of this function, we're asserting it as Response. What do you think of asserting the return type as Response and only asserting the type where we need to treat it as a MockResponse. Or maybe we can use one of the jest utility types like jest.Mocked<> so the same type works in both contexts.

[Zmatarasso]

I like your first idea better, but I get the feeling jest.Mocked is probably the easier route. I'm open to either.