feat(logger): report errors to Sentry when SENTRY_DSN is set

[juliusknorr]

Part of Euro-Office/DocumentServer#392 (stage 2).

What is Sentry? Sentry is an error tracking service. Applications send errors to it with their stack trace and context (release, service, document), and it groups repeated errors into issues with counts, first and last seen, and alerts. Self-hosted Sentry and GlitchTip speak the same protocol, so any of them works via a DSN.

Why: today errors only end up in per-service log files on each server. With this, admins and developers see which errors happen, how often, since which release and in which service, without collecting logs by hand. Reporting is opt-in and off unless SENTRY_DSN is set; tokens are scrubbed and no user data is sent by default.

• Common/sources/reporters/sentry.js: log4js appender for level error+, @sentry/node 10
• logger.js adds the appender when SENTRY_DSN is set (in code rather than via entrypoints, so pkg bundles it); disabled otherwise
• Common/sources/reporters/index.js is the only entry point: init(service) initializes all backends, appenders() returns their log4js appenders. Adding a backend = one file plus one line there
• Initialized first in DocService, FileConverter, SpellChecker, AdminPanel; events carry a service tag (docservice, converter, spellchecker, adminpanel)
• Config via SENTRY_DSN, SENTRY_ENVIRONMENT, SENTRY_RELEASE (default documentserver@<version>)
• Groups by format template (plus the stack when an Error object is passed), passes Error objects through
• Tags tenant and docId from the log context (placeholders skipped); userId and Nextcloud ids follow in stage 3
• Flushes on logger.shutdown. Unhandled rejections already reach the existing uncaughtException handlers (Node's default throw mode), so the SDK's own crash handlers are disabled. AdminPanel gets the same handler.
• Scrubs token=/access_token=/doc= params, raw JWTs, Authorization headers and bearer tokens; sendDefaultPii: false

Tested: unit test in tests/unit/sentryReporter.tests.js, plus a smoke run of the real SDK against a local sink (scrubbing, grouping and flush before exit).

AI-assisted, see Assisted-by trailer.

[chrip]

Summary

This adds opt-in Sentry error reporting: a log4js appender forwards ERROR and above to @sentry/node when SENTRY_DSN is set, with a service tag, tenant/docId tags, template-based grouping and a token scrubber. The structure is small and easy to follow, and the parts that are easy to get wrong hold up: the filtered integration names exist in the SDK, SENTRY_ENVIRONMENT is read by the SDK, and a pkg-built binary loads the SDK and delivers events. Two things need fixing before merge: reporting switches itself off after the first runtime config reload, and request cookies, headers and bodies still reach Sentry.

Verification

Claim	Reality	Status
Crash handlers dropped via OnUncaughtException / OnUnhandledRejection	Both names are in getDefaultIntegrations() of 10.75.3	✓
Bundled by pkg because it's wired in code	Built a pkg binary (@yao-pkg/pkg, node22) of a script that loads the reporter; it delivered events to a local sink	✓
SENTRY_ENVIRONMENT configures the environment	Read by the SDK itself, @sentry/node-core/build/cjs/sdk/index.js:128	✓
Flushes on logger.shutdown	Yes, but log4js.configure() also triggers it on every reload, see Blocking 1	❌
"no user data is sent by default"	Cookies, headers and body are attached to errors logged inside express requests, see Blocking 2	❌
Unit tests	9/9 pass locally	✓

Issues & Suggestions

🔴 Blocking

• Blocking 1: Sentry is shut down on every runtime config reload. Common/sources/reporters/sentry.js:81 implements the appender's shutdown as Sentry.close(2000). log4js calls appender shutdown not only from logger.shutdown() but also from log4js.configure() whenever it is already configured (log4js/lib/log4js.js:60-62). logger.configureLogger() runs again from runtimeConfigManager.handleConfigFileChange, and DocService, FileConverter and AdminPanel all run that watcher. So the first time the runtime config file changes (an admin saving config, WOPI settings or the admin password in AdminPanel), every service closes its Sentry client and silently stops reporting until restart. I reproduced it: an error logged 3 s after a second log4js.configure() never reaches the sink, and Sentry.isEnabled() returns false. Using Sentry.flush(2000) instead of close fixes it; the event is delivered and the client stays enabled. Please add a test that reconfigures and then reports. The current test at tests/unit/sentryReporter.tests.js:105 asserts that close is called.

• Blocking 2: cookies, request headers and request bodies are sent despite sendDefaultPii: false. In @sentry/node 10.x, sendDefaultPii: false (sentry.js:32) maps to a data-collection preset that only denies IP-like header and cookie names (forwarded, -ip, remote-, via, -user). It does not drop cookies, other headers or bodies. I ran the PR's reporter in an express app (the DocService stack) and logged an error inside a request handler. The event contained:

  • request.cookies with the oc<instanceid>, oc_sessionPassphrase, nc_session_id, nc_token and nc_username values unredacted. Browsers send these to DocService whenever it is served from the Nextcloud host, e.g. behind a sub-path proxy.
  • request.headers, including a custom JWT header without a Bearer prefix (the header name is configurable via services.CoAuthoring.token.inbox.header).
  • request.data with the full JSON body: user data and a WebDAV path containing the username and file name.

  scrub() only catches Authorization, Bearer and raw JWTs. A fix I verified against the same harness is to reduce event.request in beforeSend to method, URL and query string:

  if (event.request) {
    const {method, url, query_string} = event.request;
    event.request = {method, url, query_string};
  }

  Setting dataCollection: {userInfo: false, cookies: false, httpHeaders: false, httpBodies: []} removes cookies and headers but not request.data in 10.75.3, so the beforeSend strip is the reliable option. A test with a cookie and a body on the event would pin this down; the assertion at tests/unit/sentryReporter.tests.js:89 changes accordingly.

ℹ️ Minor

• Minor 3: all crashes of a service collapse into one Sentry issue. When no Error object is among the arguments, sentry.js:73 fingerprints by the template alone. The crash handlers, including the new one at AdminPanel/server/sources/server.js:184, log 'uncaughtException:%s', err.stack, so every uncaught exception becomes the same issue regardless of cause, and the event carries no structured stack trace. Adding the first line of a stack-shaped %s argument to the fingerprint in the appender would split them without changing the log output.

Verdict

Request changes. The design is sound and pkg packaging works, but Blocking 1 makes reporting stop after normal admin use, and Blocking 2 sends session cookies and request bodies, which contradicts "no user data is sent by default". Both fixes are small, and I verified each one locally.

Assisted-by: ClaudeCode:claude-opus-5-5

[juliusknorr]

Thanks @chrip

all three addressed in 1b317e5:

1. Appender shutdown now uses Sentry.flush(2000) instead of close, so reporting survives log4js reconfigures. Test asserts flush (not close) and that reporting still works afterwards; also checked against the real SDK with a double log4js.configure().
2. beforeSend reduces event.request to method, url and query string (query still token-scrubbed). Test covers cookies and a request body.
3. Stack-shaped %s arguments add their first line to the fingerprint, so uncaught exceptions split by error.

Resolved