Skip to content

fix(jobs): load the OpenTelemetry setup in every process running app code - #2973

Closed
Tobbe wants to merge 3 commits into
mainfrom
fix/2861-otel-load-in-all-processes
Closed

Tobbe wants to merge 3 commits into
mainfrom
fix/2861-otel-load-in-all-processes

Conversation

@Tobbe

@Tobbe Tobbe commented Oct 10, 2026

Copy link
Copy Markdown
Member

Fixes #2861

The OTel SDK setup file was only ever loaded by the dev API server watcher, and via --require, which cannot load an ESM file that uses top-level await.

A shared getOTelImportArgs() helper in @cedarjs/project-config resolves the built api/dist/opentelemetry.js and returns a --import=<path> entry. It is now used by:

  • the dev watcher (api-server-watch) — replacing the inline --require logic
  • the production API server with a server file (cedarjs-server api, cedar serve api)
  • the job workers (cedar jobs work|start forks)
  • the default in-process API server, which await import()s the setup file before Fastify, Prisma and the app's own modules load

The setup command now prints the flag to use when running under your own process manager.


Opened from a branch in this repo so the full CI suite runs. Original PR by @niukanen1: #2887

…code

The OTel SDK setup file was only loaded by the dev API server watcher,
and via --require, which cannot load an ESM file using top-level await.
A shared getOTelImportArgs() helper now preloads api/dist/opentelemetry.js
with --import in the production API server (both the server-file path and
the default server), the job workers, and the dev watcher. The default
in-process server imports the setup file before Fastify and Prisma load.
The setup command prints the flag to use under your own process manager.
…s serving

The setup path is now returned as a file URL so Node resolves it as an
ESM specifier on Windows too. The default API server and the both-sides
in-process server import Fastify and the server factory after the setup
file, so the Fastify instrumentation can still patch them. The bin entry
loads the api handlers lazily for the same reason. cedar serve --ud
imports the setup before the built Fetchable, cedar serve with a custom
server file preloads it on the node process for both sides, and the
setup command notice mentions the covered modes.
The lazy-handler change dropped the webHandler import, so cedarjs-server
web crashed with a ReferenceError before parsing options.
@netlify

netlify Bot commented Oct 10, 2026 •

Copy link
Copy Markdown

✅ Deploy Preview for cedarjs canceled.

Name Link
🔨 Latest commit 99c31e5
🔍 Latest deploy log https://app.netlify.com/projects/cedarjs/deploys/6aca3fc231a9180009154e3f

@github-actions github-actions Bot added this to the next-release-patch milestone Oct 10, 2026
@coderabbitai

coderabbitai Bot commented Oct 10, 2026

Copy link
Copy Markdown

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e8488a8e-7017-4ef6-83b9-d3193da73dfd

📥 Commits

Reviewing files that changed from the base of the PR and between f21b071 and 99c31e5.


📒 Files selected for processing (13)
  • packages/api-server-watch/src/serverManager.ts
  • packages/api-server/src/apiCLIConfigHandler.ts
  • packages/api-server/src/bin.ts
  • packages/api-server/src/bothCLIConfigHandler.ts
  • packages/api-server/src/serverFile.ts
  • packages/cli/src/commands/experimental/setupOpentelemetryHandler.ts
  • packages/cli/src/commands/serve.ts
  • packages/cli/src/commands/serveApiHandler.ts
  • packages/cli/src/commands/serveBothHandler.ts
  • packages/jobs/src/bins/cedar-jobs.ts
  • packages/project-config/src/__tests__/otel.test.ts
  • packages/project-config/src/index.ts
  • packages/project-config/src/otel.ts

 __________________________________________________
< Deploying the charm offensive against your bugs. >
 --------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Tobbe Tobbe closed this Oct 10, 2026
@Tobbe
Tobbe deleted the fix/2861-otel-load-in-all-processes branch October 10, 2026 13:39
@greptile-apps

greptile-apps Bot commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[High impact] Refactors OpenTelemetry preload across multiple process launchers.

Fix the combined-server command's quoting before merging; valid project paths can now prevent API startup.

Findings

  1. P1 Project paths break server startup ▶
  2. P2 Unified dev notice promises traces ▶
  3. P2 Tests change shared fixture files ▶

Summary

The PR adds a shared OpenTelemetry preload helper and uses it before API modules and job workers load.

  • The dev API watcher loads the telemetry setup with Node’s ESM import flag.
  • API servers load the telemetry setup before app modules.
  • Job workers preload telemetry, and the setup command explains manual startup.

Reviews (1) · Last reviewed commit: "fix(api-server): restore the web command..." · Reviewed by Greptile

{
name: 'api',
command: `${formatRunBinCommand('node', [path.join('dist', 'server.js'), '--apiPort', String(argv.apiPort), '--apiHost', argv.apiHost, '--apiRootPath', apiRootPath])}`,
command: `${formatRunBinCommand('node', [...getOTelImportArgs(), path.join('dist', 'server.js'), '--apiPort', String(argv.apiPort), '--apiHost', argv.apiHost, '--apiRootPath', apiRootPath])}`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Project paths break server startup

If the project directory contains shell characters, such as /srv/cedar&demo, cedar serve with OpenTelemetry enabled and a custom server now fails to start the API. The new absolute file URL goes into an unquoted shell command. The & remains in the URL, so the shell splits the command instead of passing the complete --import argument to Node.

Quote the preload argument for the shell, or launch Node with an argument array.

title: 'Notice: running under your own process manager...',
task: (_ctx: unknown, task: { output: string }) => {
task.output = [
'The setup file is loaded automatically by `cedar dev` (including `--ud`), `cedar serve api`, `cedar serve`, `cedarjs-server api` and the job workers.',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unified dev notice promises traces

The new notice says cedar dev --ud loads the setup automatically, but that command starts cedar-unified-dev, which does not import the setup. Its OpenTelemetry code only adds tracing calls to API source files; it does not register a provider or exporter. Users following this guidance can expect traces that never get exported.

Remove the --ud claim until that startup path loads the setup.

Suggested change
'The setup file is loaded automatically by `cedar dev` (including `--ud`), `cedar serve api`, `cedar serve`, `cedarjs-server api` and the job workers.',
'The setup file is loaded automatically by `cedar dev` without `--ud`, `cedar serve api`, `cedar serve`, `cedarjs-server api` and the job workers.',

Comment on lines +49 to +57
const configPath = path.join(FIXTURE_BASEDIR, 'redwood.toml')
const originalConfig = fs.readFileSync(configPath, 'utf-8')
try {
fs.writeFileSync(
configPath,
originalConfig.concat(
'\n[experimental.opentelemetry]\n\tenabled = true\n\twrapApi = true\n',
),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Tests change shared fixture files

These tests rewrite the shared empty-project/redwood.toml and remove the entire api/dist directory during cleanup. Other test files read this fixture, and the test configuration allows parallel files. Readers can therefore see the configuration while it is being rewritten, and future tests that add build files can have those files deleted.

Use a private temporary project, or mock configuration and file checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenTelemetry setup file is only loaded by the dev API server — not in production, not in job workers (and via --require in ESM apps)

2 participants