← Back to PRs

#19353: fix(diagnostics-otel): fix cross-chunk module isolation breaking even…

by nez open 2026-02-17 18:20 View on GitHub →
channel: slack channel: whatsapp-web gateway extensions: diagnostics-otel cli commands agents size: M
## Summary - The bundler emits separate chunks for the main app and plugin-sdk, each with their own module-level singletons. When jiti-loaded extensions import `onDiagnosticEvent` or `registerLogTransport` from the plugin-sdk chunk, they subscribe to a different `Set` instance than the one the main app emits into — so diagnostic events and log transports silently vanish. - Fix by storing `listeners` and `externalTransports` on `globalThis` via well-known `Symbol.for()` keys, guaranteeing all chunks share the same instance regardless of how many times the module is loaded. - Also fixes the diagnostics-otel extension: migrate deprecated `metricReader` → `metricReaders` array API, suppress auto-configured logs exporter, and add structured startup logging. ## Test plan - [ ] Rebuild container from this branch - [ ] Verify `diagnostics-otel: SDK started` appears in startup logs - [ ] Send a message to the bot and confirm traces/metrics appear in otelcol debug output - [ ] Verify data reaches OTEL dashboard (traces, metrics, logs) - [ ] Confirm no `The 'metricReader' option is deprecated` warning in logs ## AI Disclosure Built with Claude Code (Opus 4.6). Tested — verified running (otelcol received traces and metrics), and ran `pnpm build && pnpm check && pnpm test`. <!-- greptile_comment --> <h3>Greptile Summary</h3> This PR fixes a real and well-understood bug: the bundler emits separate chunks for the main app and plugin-sdk, each with their own module-level singletons. Extensions loaded via `jiti` subscribe to different `Set` instances than the main app emits into, silently dropping diagnostic events and log transports. The fix stores both singletons on `globalThis` via `Symbol.for()` keys, following the existing pattern already used in `src/web/test-helpers.ts`. It also updates the `diagnostics-otel` extension: migrates the deprecated `metricReader` → `metricReaders` array API, adds error handling around `sdk.start()`, adds structured startup logging, and suppresses auto-configured OTEL logs exporters. **Key findings:** - **Critical (test breakage):** The `@opentelemetry/api` mock in `service.test.ts` (line 34–44) does not include `diag`, `DiagConsoleLogger`, or `DiagLogLevel`, which are now imported and used unconditionally on line 51 of `service.ts`. This will cause a `TypeError` when the test runs `service.start()`, breaking the existing test suite. - `diag.setLogger(...)` is called before the `!cfg?.enabled || !otel?.enabled` early-return guard, meaning it sets a process-global OTel diagnostic logger even when the extension is disabled. This is a minor concern but worth noting. - `process.env.OTEL_LOGS_EXPORTER = "none"` is set as a permanent process-global side effect inside `start()` and is never cleared in `stop()`. It is also only set when `tracesEnabled || metricsEnabled`, so it has no effect when logs-only mode is used. - The `globalThis` singleton pattern for `listeners` and `externalTransports` is correct and matches existing codebase patterns. Test isolation via `resetDiagnosticEventsForTest()` is preserved. <h3>Confidence Score: 3/5</h3> - The core fix is sound, but the PR breaks the existing test suite due to an incomplete mock update in service.test.ts. - The `globalThis` singleton approach for both `listeners` and `externalTransports` is correct, well-precedented in the codebase, and solves the described cross-chunk isolation problem. The `diagnostics-otel` service improvements (deprecated API migration, error handling, startup logging) are straightforward improvements. However, the test mock for `@opentelemetry/api` in `service.test.ts` was not updated to include the newly-imported `diag`, `DiagConsoleLogger`, and `DiagLogLevel`. Since `vi.mock` replaces the entire module, these will be `undefined` and `diag.setLogger()` will throw a TypeError, causing the test to fail. The PR description states `pnpm test` was run successfully, but this test failure should have surfaced — worth double-checking before merge. - Pay close attention to `extensions/diagnostics-otel/src/service.test.ts` — the `@opentelemetry/api` mock needs to be updated to include `diag`, `DiagConsoleLogger`, and `DiagLogLevel`. <sub>Last reviewed commit: af1650c</sub> <!-- greptile_other_comments_section --> <sub>(4/5) You can add custom instructions or style guidelines for the agent [here](https://app.greptile.com/review/github)!</sub> <!-- /greptile_comment -->

Most Similar PRs