feat: otel and signoz implementation#1297
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughOpenTelemetry tracing and logging are integrated into the API gateway and logger service. The main application initializes the OpenTelemetry SDK, and the logger service emits logs to OpenTelemetry with enriched context. Additional dependencies for OpenTelemetry are added, and minor refactoring and formatting changes are applied across several files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Gateway (main.ts)
participant OpenTelemetry SDK (tracer.ts)
participant LoggerService
participant OTLP Exporter
Client->>API_Gateway (main.ts): Sends request
API_Gateway (main.ts)->>OpenTelemetry SDK: Initializes tracing/logging (on startup)
API_Gateway (main.ts)->>LoggerService: Handles request, logs event
LoggerService->>OpenTelemetry SDK: Emits log event with context
OpenTelemetry SDK->>OTLP Exporter: Exports traces/logs
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
apps/api-gateway/src/main.ts (1)
22-29: Consider enhancing error handling for production readiness.The try-catch implementation is good, but consider whether the application should continue if OpenTelemetry fails to start, especially in production environments.
try { await otelSDK.start(); // eslint-disable-next-line no-console console.log('OpenTelemetry SDK started successfully'); } catch (error) { // eslint-disable-next-line no-console console.error('Failed to start OpenTelemetry SDK:', error); + // Consider: Should we exit(1) in production if telemetry is critical? + if (process.env.NODE_ENV === 'production' && process.env.REQUIRE_TELEMETRY === 'true') { + process.exit(1); + } }libs/logger/src/nestjsLoggerServiceAdapter.ts (1)
44-53: Consider improving the span-aware logging implementation.The concept is good but there are opportunities for enhancement:
- More descriptive class name
- Error handling for span operations
- Structured logging with attributes
-@Injectable({ scope: Scope.TRANSIENT }) -export class MyLoggerService extends ConsoleLogger { - customLog(message: string): void { - const activeSpan = api.trace.getSpan(api.context.active()); - if (activeSpan) { - activeSpan.addEvent(message); - } - this.log(message); - } -} +@Injectable({ scope: Scope.TRANSIENT }) +export class OpenTelemetryAwareLoggerService extends ConsoleLogger { + customLog(message: string, attributes?: Record<string, any>): void { + try { + const activeSpan = api.trace.getSpan(api.context.active()); + if (activeSpan) { + activeSpan.addEvent(message, attributes); + } + } catch (error) { + // Silently handle span errors to avoid breaking application flow + console.warn('Failed to add span event:', error); + } + this.log(message); + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/api-gateway/src/main.ts(4 hunks)apps/api-gateway/src/tracer.ts(1 hunks)libs/logger/src/logger.service.ts(3 hunks)libs/logger/src/logging.interceptor.ts(1 hunks)libs/logger/src/nestjsLoggerServiceAdapter.ts(1 hunks)package.json(2 hunks)
🧰 Additional context used
🪛 ESLint
libs/logger/src/logging.interceptor.ts
[error] 12-12: Expected no linebreak before this expression.
(implicit-arrow-linebreak)
🔇 Additional comments (8)
package.json (1)
45-54: Verify OpenTelemetry package versions for security and compatibility.The comprehensive set of OpenTelemetry dependencies looks appropriate for the integration. However, ensure these versions are current and free from known vulnerabilities.
What are the latest stable versions of OpenTelemetry Node.js packages as of 2024?apps/api-gateway/src/main.ts (2)
1-1: LGTM! Proper SDK import placement.Importing the OpenTelemetry SDK at the very top ensures instrumentation is active before other modules are loaded.
44-55: LGTM! Arrow function refactor improves consistency.The middleware conversion to arrow function syntax is clean and consistent with modern JavaScript practices.
apps/api-gateway/src/tracer.ts (2)
54-61: LGTM! Proper graceful shutdown implementation.The SIGTERM handler correctly shuts down both the SDK and logger provider, ensuring telemetry data is properly flushed before process exit.
32-34: Verify the authorization header format for your telemetry backend.The
Api-Keyauthorization format should match your telemetry backend's expected format (SigNoz in this case).What is the correct authorization header format for SigNoz OTLP endpoints?Also applies to: 39-41
libs/logger/src/nestjsLoggerServiceAdapter.ts (1)
13-35: LGTM! Clean refactoring to void methods.The removal of explicit
returnstatements makes the code cleaner while maintaining the same functionality.libs/logger/src/logger.service.ts (2)
66-68: LGTM - Clean delegation pattern.The
startProfilemethod correctly delegates to the underlying logger implementation.
8-8: Verify cross-module dependency architecture.The import of
otelLoggerfrom../../../apps/api-gateway/src/tracercreates a direct dependency from a library (libs/logger) to an application module (apps/api-gateway). This violates typical layered architecture principles where libraries should not depend on specific applications.Consider refactoring to inject the otel logger as a dependency or use a shared module to avoid tight coupling between library and application layers.
#!/bin/bash # Search for other cross-module dependencies between libs and apps rg --type ts "from.*apps/" libs/
| private emitToOtel(severityText: string, message: string | Error, data?: LogData): void { | ||
| const correlationId = data?.correlationId || this.contextStorageService.getContextId(); | ||
| otelLogger.emit({ | ||
| body: `${correlationId} ${'string' === typeof message ? message : message.message}`, | ||
| severityText, | ||
| attributes: { | ||
| app: data?.app || this.app, | ||
| organization: data?.organization || this.organization, | ||
| context: data?.context || this.context, | ||
| sourceClass: data?.sourceClass || this.sourceClass, | ||
| correlationId, | ||
| microservice: this.microserviceName, | ||
| ...(data | ||
| ? { | ||
| ...data, | ||
| ...(data.error | ||
| ? { | ||
| error: | ||
| 'string' === typeof data.error | ||
| ? data.error | ||
| : data.error instanceof Error | ||
| ? { | ||
| name: data.error.name, | ||
| message: data.error.message, | ||
| stack: data.error.stack | ||
| } | ||
| : 'object' === typeof data.error | ||
| ? JSON.parse(JSON.stringify(data.error)) | ||
| : String(data.error) | ||
| } | ||
| : {}) | ||
| } | ||
| : {}) | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Robust error normalization but verify edge cases.
The emitToOtel method implements comprehensive error normalization logic. However, there are a few areas to consider:
- Line 97:
JSON.parse(JSON.stringify(data.error))could throw if the object contains circular references or non-serializable properties. - The method doesn't handle potential failures of
otelLogger.emit().
Consider adding safer error handling:
private emitToOtel(severityText: string, message: string | Error, data?: LogData): void {
+ try {
const correlationId = data?.correlationId || this.contextStorageService.getContextId();
otelLogger.emit({
body: `${correlationId} ${'string' === typeof message ? message : message.message}`,
severityText,
attributes: {
app: data?.app || this.app,
organization: data?.organization || this.organization,
context: data?.context || this.context,
sourceClass: data?.sourceClass || this.sourceClass,
correlationId,
microservice: this.microserviceName,
...(data
? {
...data,
...(data.error
? {
error:
'string' === typeof data.error
? data.error
: data.error instanceof Error
? {
name: data.error.name,
message: data.error.message,
stack: data.error.stack
}
: 'object' === typeof data.error
- ? JSON.parse(JSON.stringify(data.error))
+ ? this.safeStringify(data.error)
: String(data.error)
}
: {})
}
: {})
}
});
+ } catch (error) {
+ // Silently fail if telemetry emission fails
+ }
}
+
+private safeStringify(obj: any): string {
+ try {
+ return JSON.stringify(obj);
+ } catch {
+ return '[Circular or Non-Serializable Object]';
+ }
+}📝 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.
| private emitToOtel(severityText: string, message: string | Error, data?: LogData): void { | |
| const correlationId = data?.correlationId || this.contextStorageService.getContextId(); | |
| otelLogger.emit({ | |
| body: `${correlationId} ${'string' === typeof message ? message : message.message}`, | |
| severityText, | |
| attributes: { | |
| app: data?.app || this.app, | |
| organization: data?.organization || this.organization, | |
| context: data?.context || this.context, | |
| sourceClass: data?.sourceClass || this.sourceClass, | |
| correlationId, | |
| microservice: this.microserviceName, | |
| ...(data | |
| ? { | |
| ...data, | |
| ...(data.error | |
| ? { | |
| error: | |
| 'string' === typeof data.error | |
| ? data.error | |
| : data.error instanceof Error | |
| ? { | |
| name: data.error.name, | |
| message: data.error.message, | |
| stack: data.error.stack | |
| } | |
| : 'object' === typeof data.error | |
| ? JSON.parse(JSON.stringify(data.error)) | |
| : String(data.error) | |
| } | |
| : {}) | |
| } | |
| : {}) | |
| } | |
| }); | |
| } | |
| private emitToOtel(severityText: string, message: string | Error, data?: LogData): void { | |
| try { | |
| const correlationId = data?.correlationId || this.contextStorageService.getContextId(); | |
| otelLogger.emit({ | |
| body: `${correlationId} ${'string' === typeof message ? message : message.message}`, | |
| severityText, | |
| attributes: { | |
| app: data?.app || this.app, | |
| organization: data?.organization || this.organization, | |
| context: data?.context || this.context, | |
| sourceClass: data?.sourceClass || this.sourceClass, | |
| correlationId, | |
| microservice: this.microserviceName, | |
| ...(data | |
| ? { | |
| ...data, | |
| ...(data.error | |
| ? { | |
| error: | |
| 'string' === typeof data.error | |
| ? data.error | |
| : data.error instanceof Error | |
| ? { | |
| name: data.error.name, | |
| message: data.error.message, | |
| stack: data.error.stack | |
| } | |
| : 'object' === typeof data.error | |
| ? this.safeStringify(data.error) | |
| : String(data.error) | |
| } | |
| : {}) | |
| } | |
| : {}) | |
| } | |
| }); | |
| } catch (error) { | |
| // Silently fail if telemetry emission fails | |
| } | |
| } | |
| private safeStringify(obj: any): string { | |
| try { | |
| return JSON.stringify(obj); | |
| } catch { | |
| return '[Circular or Non-Serializable Object]'; | |
| } | |
| } |
🤖 Prompt for AI Agents
In libs/logger/src/logger.service.ts between lines 70 and 105, the emitToOtel
method's error normalization uses JSON.parse(JSON.stringify(data.error)), which
can throw on circular or non-serializable objects, and otelLogger.emit() is
called without error handling. To fix this, replace the JSON serialization with
a safer deep clone or a utility that handles circular references, and wrap the
otelLogger.emit() call in a try-catch block to handle and log any exceptions
gracefully without crashing the application.
| this.contextStorageService.set('x-correlation-id', newContextId); | ||
| this.contextStorageService.setContextId(newContextId); | ||
| } | ||
| //TODO: Uncomment this when user context needs to be set |
There was a problem hiding this comment.
Do we need this commented code?
There was a problem hiding this comment.
It's required for user context, but we can maintain it in the doc or somewhere else
| export class MyLoggerService extends ConsoleLogger { | ||
| customLog(message: string): void { | ||
| const activeSpan = api.trace.getSpan(api.context.active()); | ||
| if (activeSpan) { | ||
| activeSpan.addEvent(message); | ||
| } | ||
| this.log(message); | ||
| } | ||
| } |
There was a problem hiding this comment.
Where are we exactly using this class?
There was a problem hiding this comment.
It was for custom span, currently we are not supporting this. Removed
There was a problem hiding this comment.
It was for custom span, currently we are not supporting this. Removed
| public startProfile(id: string) : void { | ||
| this.logger.startProfile(id); | ||
| } |
There was a problem hiding this comment.
Are we sure, we won't need this, because the interface we are implementing, has a startProfile function signature?
There was a problem hiding this comment.
The startProfile method remains in the file. Only its position within the file has changed.
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
tipusinghaw
force-pushed
the
feat/otel-and-signoz-integration-platform
branch
from
ba1be12 to
7d40bb0
Compare
Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com>
|
* feat: otel and signoz implementation Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> * fix:resolve suggested comments Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> * fix:resolve suggested for logging Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> * feat: added flag for local setup Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> * refactor: updated env sample and demo Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> --------- Signed-off-by: Tipu_Singh <tipu.singh@ayanworks.com> Signed-off-by: Ankita Patidar <ankita.patidar@ayanworks.com>
3 participants
What
Summary by CodeRabbit
New Features
Improvements
Dependencies