feat: add otel namespace to logging package #2075
Conversation
WalkthroughRemoves module-level OpenTelemetry mixins and side-effect OTEL imports from apps, adds a reusable OTEL integration in the logging package (collectOtel and createOtelLogger), and updates applications to instantiate OTEL-aware loggers via the new factory instead of global mixins. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (console/server/container)
participant Factory as createOtelLogger()
participant OtelAPI as @opentelemetry/api
participant Logger as LoggerService
rect rgb(250,240,240)
Note over App,Logger: Old flow (module-level mixin)
App->>App: import "./open-telemetry" (side-effect)
App->>Logger: LoggerService.mixin = collect span
App->>Logger: new LoggerService(...)
Logger-->>App: logger instance (global mixin applied)
end
rect rgb(240,250,240)
Note over App,Factory: New flow (factory)
App->>Factory: createOtelLogger(options)
Factory->>OtelAPI: context.active() / trace.getSpan(...)
OtelAPI-->>Factory: current span / spanContext
Factory->>Logger: new LoggerService({...options, mixin: collectOtel})
Logger-->>Factory: pre-configured logger
Factory-->>App: return logger instance
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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). (9)
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. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/console.ts(0 hunks)apps/api/src/core/providers/logging.provider.ts(1 hunks)apps/api/src/open-telemetry.ts(0 hunks)apps/api/src/server.ts(0 hunks)apps/provider-proxy/src/container.ts(3 hunks)apps/provider-proxy/src/server.ts(0 hunks)apps/provider-proxy/src/setupOpenTelemetry.ts(0 hunks)packages/logging/package.json(2 hunks)packages/logging/src/otel.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/provider-proxy/src/setupOpenTelemetry.ts
- apps/provider-proxy/src/server.ts
- apps/api/src/console.ts
- apps/api/src/server.ts
- apps/api/src/open-telemetry.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
packages/logging/src/otel.tsapps/provider-proxy/src/container.tsapps/api/src/core/providers/logging.provider.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
packages/logging/src/otel.tsapps/provider-proxy/src/container.tsapps/api/src/core/providers/logging.provider.ts
🧬 Code graph analysis (3)
packages/logging/src/otel.ts (1)
packages/logging/src/services/logger/logger.service.ts (1)
LoggerOptions(18-21)
apps/provider-proxy/src/container.ts (2)
packages/logging/src/otel.ts (1)
createOtelLogger(20-22)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
createCertificateValidatorInstrumentation(84-99)
apps/api/src/core/providers/logging.provider.ts (1)
apps/api/src/core/services/execution-context/execution-context.service.ts (1)
context(18-26)
⏰ 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). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (7)
apps/provider-proxy/src/container.ts (2)
3-3: LGTM: Import added for centralized OTEL logger.The import correctly references the new
@akashnetwork/logging/otelexport introduced in this PR.
16-16: LGTM: Logger instantiations correctly migrated tocreateOtelLogger.All logger instances now use the centralized OTEL-aware factory function, eliminating code duplication and ensuring consistent trace context injection.
Also applies to: 32-32, 35-36
apps/api/src/core/providers/logging.provider.ts (1)
3-3: LGTM: OpenTelemetry API imported for mixin.The import correctly brings in the necessary APIs for trace context collection.
packages/logging/package.json (2)
14-14: LGTM: Export added for OTEL logger factory.The export correctly maps
./otelto the newotel.tsmodule, enabling consumers to import the centralized OTEL logger creation function.
38-38: LGTM: Peer dependency correctly configured.The
@opentelemetry/apipeer dependency version^1.9.0is current and stable, and the optional flag appropriately reflects that OTEL integration is opt-in. Based on learnings.Also applies to: 44-46
packages/logging/src/otel.ts (2)
1-4: LGTM: Imports are correct.The necessary OpenTelemetry APIs and logging service types are correctly imported.
20-22: LGTM: Factory function correctly creates OTEL-aware logger.The
createOtelLoggerfunction correctly instantiates aLoggerServicewith thecollectOtelmixin, ensuring trace context is automatically injected into all log entries.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/provider-proxy/src/container.ts (2)
15-15: DRY the logger creation with a tiny helper.Avoid repeating the
isLoggingDisabled ? undefined : createOtelLogger({ name })ternary; define once and reuse.const isLoggingDisabled = process.env.NODE_ENV === "test"; + const mkLogger = (name: string) => (isLoggingDisabled ? undefined : createOtelLogger({ name })); + const wsStats = new WebsocketStats(); - const appLogger = isLoggingDisabled ? undefined : createOtelLogger({ name: "app" }); + const appLogger = mkLogger("app"); @@ - isLoggingDisabled ? undefined : createCertificateValidatorInstrumentation(createOtelLogger({ name: "cert-validator" })) + (mkLogger("cert-validator") + ? createCertificateValidatorInstrumentation(mkLogger("cert-validator")!) + : undefined) @@ - const wsLogger = isLoggingDisabled ? undefined : createOtelLogger({ name: "ws" }); - const httpLogger = isLoggingDisabled ? undefined : createOtelLogger({ name: "http" }); + const wsLogger = mkLogger("ws"); + const httpLogger = mkLogger("http");Also applies to: 31-31, 34-35
34-36: Confirm interceptor constructor tolerates undefined logger; guard if not.If
HttpLoggerIntercepterexpects a concrete logger, passingundefinedwhen tests run will throw. Guard creation or make the parameter optional.- const httpLoggerInterceptor = new HttpLoggerIntercepter(httpLogger); + const httpLoggerInterceptor = httpLogger + ? new HttpLoggerIntercepter(httpLogger) + : undefined as unknown as HttpLoggerIntercepter; // if type must exist in the container, consider a Noop interceptor insteadIf types don’t allow
undefined, consider exporting a Noop interceptor from the logging package and use that in tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/provider-proxy/src/container.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/provider-proxy/src/container.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/provider-proxy/src/container.ts
🧬 Code graph analysis (1)
apps/provider-proxy/src/container.ts (2)
packages/logging/src/otel.ts (1)
createOtelLogger(20-22)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
createCertificateValidatorInstrumentation(84-99)
⏰ 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). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (1)
apps/provider-proxy/src/container.ts (1)
31-31: Verify OTEL resource initialization and add explicit service/resource attributes if needed.The review comment's concern is valid. Investigation confirms that:
- The OTEL mixin (
collectOtel) adds only span context (trace/span IDs), not service/resource metadata- Service names passed to loggers as options (
{ name: "app" }) are not exposed as OTEL resource attributes- No explicit OpenTelemetry Resource configuration is present in the codebase
While
@opentelemetry/auto-instrumentations-nodeis a dependency and may provide default resource metadata at runtime, this is not explicitly configured. To ensure consistent service/resource metadata (service.name, service.version) across components for proper log correlation, the developer should:
- Verify that auto-instrumentations-node provides adequate resource initialization, OR
- Explicitly configure an OTEL Resource with service.name, service.version, and other identifying attributes during app initialization
ritoban23
force-pushed
the
feat/add-otel-namespace-1801
branch
from
9d4654c to
9fe60c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/provider-proxy/src/container.ts (1)
2-2: Critical issues from previous review remain unaddressed.The past review comments on this line identified two critical issues that appear to still be valid:
Unconditional import of optional peer —
packages/logging/src/otel.tsimports@opentelemetry/apiunconditionally, but it's declared as an optional peer dependency. This will cause runtime failures if the peer is not installed.Missing OTEL SDK initialization — The removal of
setupOpenTelemetry.tsleft no SDK initialization in provider-proxy. Without initializing an OTEL SDK (NodeSDK, BasicTracerProvider, etc.),trace.getSpan(context.active())returns null and tracing features are non-functional.Please address both issues or confirm that SDK initialization occurs elsewhere.
🧹 Nitpick comments (2)
apps/api/src/core/providers/logging.provider.ts (1)
6-8: Consider removing explanatory comments.The comments explain what the code does, which may be unnecessary for expert developers. The code is self-documenting:
LoggerServiceOriginal.mixin = collectOtelclearly sets up the OTEL mixin.As per coding guidelines, consider this diff:
-// Set up OpenTelemetry mixin for all LoggerService instances -// This collects trace information and adds it to log entries LoggerServiceOriginal.mixin = collectOtel; -apps/provider-proxy/src/container.ts (1)
15-35: Consider extracting repeated ternary pattern.The pattern
isLoggingDisabled ? undefined : createOtelLogger({ name: "..." })is repeated at lines 15, 31, 34, and 35, creating minor duplication.Consider extracting a helper:
const createLogger = (name: string) => isLoggingDisabled ? undefined : createOtelLogger({ name });Then use it:
- const appLogger = isLoggingDisabled ? undefined : createOtelLogger({ name: "app" }); + const appLogger = createLogger("app");Apply similarly to
wsLogger,httpLogger, and the cert-validator logger.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/console.ts(0 hunks)apps/api/src/core/providers/logging.provider.ts(1 hunks)apps/api/src/open-telemetry.ts(0 hunks)apps/api/src/server.ts(0 hunks)apps/provider-proxy/src/container.ts(3 hunks)apps/provider-proxy/src/server.ts(0 hunks)apps/provider-proxy/src/setupOpenTelemetry.ts(0 hunks)packages/logging/package.json(2 hunks)packages/logging/src/otel.ts(1 hunks)
💤 Files with no reviewable changes (5)
- apps/provider-proxy/src/setupOpenTelemetry.ts
- apps/provider-proxy/src/server.ts
- apps/api/src/server.ts
- apps/api/src/console.ts
- apps/api/src/open-telemetry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/logging/src/otel.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/providers/logging.provider.tsapps/provider-proxy/src/container.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/providers/logging.provider.tsapps/provider-proxy/src/container.ts
🧬 Code graph analysis (2)
apps/api/src/core/providers/logging.provider.ts (1)
packages/logging/src/otel.ts (1)
collectOtel(10-13)
apps/provider-proxy/src/container.ts (2)
packages/logging/src/otel.ts (1)
createOtelLogger(20-22)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
createCertificateValidatorInstrumentation(84-99)
⏰ 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). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
|
Nice, thank you for contribution! |
Creates a reusable OpenTelemetry logger configuration to eliminate code duplication between provider-proxy and api apps. Changes: - Add @akashnetwork/logging/otel export with createOtelLogger function - Add @opentelemetry/api as optional peer dependency - Refactor provider-proxy to use createOtelLogger - Refactor api to set global OTEL mixin in logging.provider.ts - Remove apps/provider-proxy/src/setupOpenTelemetry.ts - Remove apps/api/src/open-telemetry.ts
stalniy
force-pushed
the
feat/add-otel-namespace-1801
branch
from
9fe60c7 to
3949407
Compare
|
|
||
| // Set up OpenTelemetry mixin for all LoggerService instances | ||
| // This collects trace information and adds it to log entries | ||
| LoggerServiceOriginal.mixin = collectOtel; |
There was a problem hiding this comment.
issue(blocking): this needs to be moved back to open-telemetry.ts file in api app because this app imports and uses instances of LoggerService in other files. So, please restore open-telemetry.ts file in api for now (you can just do LoggerService.mixin = collectOtel;). SO, there may be the case when LoggerService is created before logging.provider.ts is required. In result, those logger instances won't use otel.
And in the follow up PR, you can replace LoggerService instances either with injected LoggerService from DI or with createOtelLogger
There was a problem hiding this comment.
let's move this file to a more appropriate location. There's utils dir for instance.
3 participants
Add OTEL Namespace to Logging Package
Creates a reusable OpenTelemetry logger configuration to eliminate code duplication between provider-proxy and api apps.
Changes
@akashnetwork/logging/otelexport withcreateOtelLoggerfunction@opentelemetry/apias optional peer dependencycreateOtelLoggerapps/provider-proxy/src/setupOpenTelemetry.tsapps/api/src/open-telemetry.tsBenefits
Closes #1801
Summary by CodeRabbit
Refactor
New Features
Chores