-
Notifications
You must be signed in to change notification settings - Fork 19
Update OTel #1404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Update OTel #1404
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Docker base imageDockerfile |
Updated base image from node:24.9.0-trixie-slim to node:24.11.0-trixie-slim. |
Dependenciespackage.json |
Added @fastify/otel; upgraded multiple packages including Lokalisé packages, OpenTelemetry packages, fastify (5.6.1→5.6.2), opinionated-machine (4.0.0→5.0.1), biome, universal-testing-utils, and other minor bumps. |
OpenTelemetry / Fastify instrumentationsrc/otel.ts |
Imported FastifyOtelInstrumentation; introduced module-global isInstrumentationRegistered guard; enablement now skips when NODE_ENV === 'test' or OPEN_TELEMETRY_ENABLED is 'false'; register via registerOnInitialization with ignorePaths; set/reset registration flag on init/shutdown. |
Server startup refactorsrc/server.ts, src/serverInternal.ts |
src/server.ts now dynamically imports src/serverInternal.ts and awaits startServer(); new startServer() resolves config via executeAndHandleGlobalErrors(getConfig), builds the app with monitoring/healthchecks/queues/jobs enabled, starts HTTP server, and logs/exits on fatal error. |
Testssrc/app.e2e.spec.ts |
Updated /documentation redirect expectation from HTTP 302 to 301. |
Test config / coveragevitest.config.mts |
Added src/otel.ts to coverage exclusion list. |
Sequence Diagram(s)
sequenceDiagram
participant Entrypoint as src/server.ts
participant Internal as src/serverInternal.ts
participant OTEL as src/otel.ts
participant AppFactory as App factory
participant Node as Node Runtime
rect rgba(220,235,255,0.5)
Entrypoint->>Internal: dynamic import
Entrypoint->>Internal: await startServer()
end
rect rgba(235,250,220,0.5)
Internal->>OTEL: initialize (guarded by NODE_ENV and module flag)
OTEL-->>Internal: instrumentation ready or skipped
Internal->>AppFactory: build app (monitoring, healthchecks, queues, jobs)
AppFactory-->>Internal: app instance
Internal->>Node: start HTTP server (listen)
Node-->>Internal: listening / success
end
rect rgba(255,230,230,0.4)
note over Internal,Node: on failure -> log resolved error and process.exit(1)
end
sequenceDiagram
participant App as Fastify App
participant Global as isInstrumentationRegistered
participant FastifyOtel as `@fastify/otel` (FastifyOtelInstrumentation)
participant OTELSDK as OpenTelemetry SDK
participant AutoInstr as Node Auto-Instrumentations
App->>Global: check isInstrumentationRegistered & env flags
alt register
App->>FastifyOtel: registerOnInitialization(register, ignorePaths)
FastifyOtel-->>App: registered
App->>Global: set isInstrumentationRegistered = true
else skip
App-->>Global: skip registration
end
App->>OTELSDK: initialize SDK (auto-instr with Fastify disabled)
OTELSDK->>AutoInstr: auto-instrumentations (Fastify omitted)
OTELSDK-->>App: SDK active (Fastify spans via FastifyOtel)
note right of FastifyOtel: on shutdown -> reset isInstrumentationRegistered = false
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Files/areas needing extra attention:
src/otel.ts: registration guard correctness, environment gating (NODE_ENV/OPEN_TELEMETRY_ENABLED), lifecycle (set/reset) andignorePathssemantics.src/serverInternal.ts: config resolution viaexecuteAndHandleGlobalErrors(getConfig), feature flags used when building app, and process exit behavior on failure.package.json: major version bumps (Lokalisé / OpenTelemetry / opinionated packages) that may introduce breaking changes.src/app.e2e.spec.ts: ensure test expectation change matches deployed redirect behavior.Dockerfile: image bump implications for native dependencies/build toolchain.
Pre-merge checks and finishing touches
❌ Failed checks (1 warning, 1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Title check | ❓ Inconclusive | The title 'Update OTel' is vague and generic. While it references OpenTelemetry (OTel) which is part of the changes, the PR encompasses much broader work including Docker image updates, major dependency upgrades (Lokalisé packages, Fastify, testing utilities), refactoring of server initialization logic, and instrumentation changes. The title does not convey the scope or primary changes of this substantial PR. | Use a more descriptive title that captures the main scope, such as 'Upgrade dependencies and refactor server initialization' or 'Update OTel instrumentation and major dependencies' to better reflect the multi-faceted nature of the changes. |
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
chore/deps-11-10
📜 Recent review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/otel.ts(3 hunks)vitest.config.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vitest.config.mts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (24.x)
Comment @coderabbitai help to get the list of available commands and usage tips.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/server.ts(1 hunks)src/serverInternal.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/serverInternal.ts (2)
src/infrastructure/config.ts (1)
getConfig(87-92)src/app.ts (1)
getApp(84-337)
🔇 Additional comments (2)
src/server.ts (1)
4-6: LGTM! Clean refactoring of startup logic.The delegation to
serverInternal.tsappropriately separates concerns and maintains the correct initialization order (otel first, then server startup).src/serverInternal.ts (1)
14-38: LGTM! Well-structured startup logic with proper error handling.The startup sequence correctly:
- Resolves configuration with error handling
- Enables all feature flags (monitoring, healthchecks, queues, workers, periodic jobs)
- Starts the HTTP server with custom listen text
- Handles startup failures with logging and graceful exit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
vitest.config.mts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (24.x)
Summary by CodeRabbit
Chores
Refactor
Tests