fix: Add otel to event worker #441
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe changes integrate OpenTelemetry into the event worker service and the web service. Multiple new dependencies are added to the Changes
Sequence Diagram(s)sequenceDiagram
participant App as "Event Worker App"
participant Index as "index.ts"
participant Instr as "instrumentation.ts"
participant InstrNode as "instrumentation-node.ts"
App->>Index: Start application
Index->>Instr: Call await register()
Instr->>InstrNode: If NEXT_RUNTIME=="nodejs", dynamically import
InstrNode-->>Instr: Complete SDK initialization
Instr-->>Index: Return registration complete
Index->>App: Initialize worker instances
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
apps/event-worker/src/instrumentation.ts (2)
1-4: Consider adding braces for better code maintainabilityThe conditional block without braces is valid, but adding them would improve code clarity and prevent future bugs if additional statements are added later.
export async function register() { - if (process.env.NEXT_RUNTIME === "nodejs") - await import("./instrumentation-node.js"); + if (process.env.NEXT_RUNTIME === "nodejs") { + await import("./instrumentation-node.js"); + } }
1-4: Add error handling for dynamic importThe dynamic import could fail if the module doesn't exist or has errors. Consider adding error handling to gracefully handle such failures.
export async function register() { if (process.env.NEXT_RUNTIME === "nodejs") - await import("./instrumentation-node.js"); + try { + await import("./instrumentation-node.js"); + } catch (error) { + console.error("Failed to load OpenTelemetry instrumentation:", error); + // Optionally: throw error if you want to prevent app from starting with failed instrumentation + } }apps/event-worker/src/index.ts (1)
10-11: Consider adding error handling for the register callIf the OpenTelemetry registration fails, it could potentially prevent the application from starting. Consider adding error handling to ensure the application can still function without telemetry.
-await register(); +try { + await register(); +} catch (error) { + logger.error("Failed to register OpenTelemetry:", error); + // Application can continue without telemetry +}apps/event-worker/src/instrumentation-node.ts (2)
43-45: Ensure consistency in service name referencesThe service name "ctrlplane/event-worker" appears in two places. Consider extracting it to a constant to ensure consistency and ease of maintenance.
+const SERVICE_NAME = "ctrlplane/event-worker"; const sdk = new NodeSDK({ resource: new Resource({ - [ATTR_SERVICE_NAME]: "ctrlplane/event-worker", + [ATTR_SERVICE_NAME]: SERVICE_NAME, }), // ... instrumentations: [ getNodeAutoInstrumentations({ // ... "@opentelemetry/instrumentation-winston": { enabled: true, logHook: (span, record) => { - record["resource.service.name"] = "ctrlplane/event-worker"; + record["resource.service.name"] = SERVICE_NAME; }, }, // ...
52-57: Use logger instead of console.logSince the application uses a logger (visible in index.ts), consider using it here instead of console.log/console.error for consistency.
+import { logger } from "@ctrlplane/logger"; try { sdk.start(); - console.log("Tracing initialized"); + logger.info("OpenTelemetry tracing initialized"); } catch (error) { - console.error("Error initializing tracing", error); + logger.error("Error initializing OpenTelemetry tracing", error); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/event-worker/package.json(1 hunks)apps/event-worker/src/index.ts(1 hunks)apps/event-worker/src/instrumentation-node.ts(1 hunks)apps/event-worker/src/instrumentation.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
apps/event-worker/src/instrumentation.tsapps/event-worker/src/index.tsapps/event-worker/src/instrumentation-node.ts
🧬 Code Definitions (1)
apps/event-worker/src/index.ts (1)
apps/event-worker/src/instrumentation.ts (1)
register(1-4)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (2)
apps/event-worker/src/index.ts (1)
3-3: LGTM: Good import of the register functionThe import statement correctly references the instrumentation.js file.
apps/event-worker/src/instrumentation-node.ts (1)
13-50: Service configuration looks good with appropriately selected instrumentationsThe OpenTelemetry SDK is properly configured with the correct service name and appropriate instrumentations enabled/disabled based on the event worker's requirements.
| "@opentelemetry/auto-instrumentations-node": "^0.52.1", | ||
| "@opentelemetry/sdk-node": "^0.54.2", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider aligning package versions
The two OpenTelemetry packages have different version ranges (0.52.1 vs 0.54.2). Consider aligning them if possible to ensure compatibility and make maintenance easier.
🌐 Web query:
Are @opentelemetry/auto-instrumentations-node@0.52.1 and @opentelemetry/sdk-node@0.54.2 compatible versions?
💡 Result:
The compatibility between @opentelemetry/auto-instrumentations-node@0.52.1 and @opentelemetry/sdk-node@0.54.2 is not guaranteed due to potential breaking changes in the experimental 0.x release phase, where minor version increments often introduce compatibility risks. Here's a structured analysis:
Key Considerations
-
Experimental Phase Versioning:
Pre-1.0 versions like 0.52.x and 0.54.x follow OpenTelemetry's versioning rules where minor versions (e.g., 0.52 → 0.54) may include breaking changes[9][10]. -
Breaking Changes in 0.52.0:
The0.52.0release introduced significant breaking changes, such as:- Simplified
registerInstrumentations()API requiring instrumentations to be passed as an array[2]. - Removal of
convert()from exporters and changes to metric configuration[2].
While
0.52.1fixes bugs, it inherits these breaking changes from0.52.0[2]. - Simplified
-
SDK 0.54.x Context:
SDK0.54.2is two minor versions ahead of0.52.1. Given the experimental nature of the releases, subsequent versions like0.54.xmay further modify APIs or dependencies (e.g., environment variable handling or instrumentation registration), potentially creating incompatibilities with older auto-instrumentations packages[2][9].
Recommendation
- Use Aligned Minor Versions: Pair
@opentelemetry/sdk-node@0.54.xwith auto-instrumentations from the same minor version range (e.g.,0.54.x). - Check Release Notes: Review the changelogs for SDK
0.54.0–0.54.2to identify specific breaking changes that might affect0.52.1[3][2]. - Update Auto-Instrumentations: If upgrading the SDK, consider updating auto-instrumentations to a version tested with
0.54.x(e.g.,0.54.xor later)[4][7].
In summary, direct compatibility is unlikely without explicit confirmation from release notes. Aligning minor versions minimizes risks during the experimental pre-1.0 phase.
Citations:
- 1: https://www.npmjs.com/package/@opentelemetry/api
- 2: https://github.com/open-telemetry/opentelemetry-js/blob/main/experimental/CHANGELOG.md
- 3: https://github.com/open-telemetry/opentelemetry-js/releases
- 4: https://www.npmjs.com/package/@opentelemetry/sdk-node/v/0.51.0?activeTab=versions
- 5: https://docs.splunk.com/observability/gdi/get-data-in/application/nodejs/nodejs-otel-requirements.html
- 6: https://udd.debian.org/cgi-bin/ubuntupackages.cgi
- 7: https://opentelemetry.io/docs/languages/js/
- 8: https://www.npmjs.com/package/@opentelemetry/sdk-node/v/0.23.1-alpha.18?activeTab=versions
- 9: https://opentelemetry.io/docs/specs/otel/versioning-and-stability/
- 10: https://cloudsmith.com/navigator/npm/@opentelemetry/instrumentation
Attention: Align OpenTelemetry Package Versions for Compatibility
The current configuration in apps/event-worker/package.json uses:
"@opentelemetry/auto-instrumentations-node": "^0.52.1""@opentelemetry/sdk-node": "^0.54.2"
Due to the experimental nature of pre-1.0 releases, minor version differences (e.g., 0.52.x vs. 0.54.x) can introduce breaking changes. This compatibility gap may lead to issues in instrumentation registration and API usage. To minimize risks, please consider updating the auto-instrumentations package to the 0.54.x series (or otherwise align both packages' minor versions) and review the release notes for any pertinent breaking changes.
| try { | ||
| sdk.start(); | ||
| console.log("Tracing initialized"); | ||
| } catch (error) { | ||
| console.error("Error initializing tracing", error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider graceful shutdown of the OpenTelemetry SDK
The SDK is started correctly, but there's no mechanism to shut it down gracefully when the application terminates. Consider adding a shutdown function that can be called during the application's shutdown process.
try {
sdk.start();
console.log("Tracing initialized");
+
+ // Register shutdown handler
+ process.on('beforeExit', async () => {
+ await sdk.shutdown();
+ console.log("Tracing terminated");
+ });
} catch (error) {
console.error("Error initializing tracing", error);
}Alternatively, you could export a shutdown function that can be called from the main shutdown handler in index.ts:
try {
sdk.start();
console.log("Tracing initialized");
} catch (error) {
console.error("Error initializing tracing", error);
}
+export async function shutdownTracing() {
+ return sdk.shutdown();
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| sdk.start(); | |
| console.log("Tracing initialized"); | |
| } catch (error) { | |
| console.error("Error initializing tracing", error); | |
| } | |
| try { | |
| sdk.start(); | |
| console.log("Tracing initialized"); | |
| // Register shutdown handler | |
| process.on('beforeExit', async () => { | |
| await sdk.shutdown(); | |
| console.log("Tracing terminated"); | |
| }); | |
| } catch (error) { | |
| console.error("Error initializing tracing", error); | |
| } |
| try { | |
| sdk.start(); | |
| console.log("Tracing initialized"); | |
| } catch (error) { | |
| console.error("Error initializing tracing", error); | |
| } | |
| try { | |
| sdk.start(); | |
| console.log("Tracing initialized"); | |
| } catch (error) { | |
| console.error("Error initializing tracing", error); | |
| } | |
| export async function shutdownTracing() { | |
| return sdk.shutdown(); | |
| } |
Signed-off-by: Aditya Choudhari <48932219+adityachoudhari26@users.noreply.github.com>
…entation-event-worker
Summary by CodeRabbit