[Monitor OpenTelemetry Exporter] Fix Dynamic Import #36085
Conversation
JacksonWeber
requested review from
Karlie-777,
MSNev,
lzchen and
jeremydvoss
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR addresses circular dependency issues in the monitor-opentelemetry-exporter by refactoring the initialization of customer SDK stats metrics and statsbeat exporter to use dynamic imports and lazy initialization patterns.
- Converted synchronous singleton pattern to asynchronous with dynamic imports
- Implemented lazy initialization for HTTP sender to break circular dependencies
- Updated test mocks to handle async getInstance method
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| customerSDKStats.ts | Refactored singleton to use async pattern with dynamic import of AzureMonitorStatsbeatExporter |
| statsbeatExporter.ts | Implemented lazy initialization of HttpSender using dynamic imports |
| baseSender.ts | Updated to asynchronously initialize CustomerSDKStatsMetrics with proper error handling |
| customerSDKStats.spec.ts | Updated test to handle async getInstance method |
| baseSender.spec.ts | Updated mock to return Promise for getInstance method |
| private retryTimer: NodeJS.Timeout | null; | ||
| private networkStatsbeatMetrics: NetworkStatsbeatMetrics | undefined; | ||
| private customerSDKStatsMetrics: CustomerSDKStatsMetrics | undefined; | ||
| private customerSDKStatsMetrics: any; |
There was a problem hiding this comment.
Using any type reduces type safety. Consider creating a proper interface or using a union type with undefined to maintain type safety while accommodating the async initialization pattern.
Copilot uses AI. Check for mistakes.
| private _sender: any; | ||
| private _senderOptions: any; |
There was a problem hiding this comment.
Using any types reduces type safety. Consider importing the HttpSender type separately from the implementation, or define proper interfaces for these properties to maintain type safety.
Copilot uses AI. Check for mistakes.
| /** | ||
| * Lazily initialize the sender to avoid circular dependency | ||
| */ | ||
| private async _getSender(): Promise<any> { |
There was a problem hiding this comment.
The return type should be more specific than any. Consider importing the HttpSender type or defining a proper interface to maintain type safety.
Copilot uses AI. Check for mistakes.
| import("../../export/statsbeat/customerSDKStats.js") | ||
| .then(({ CustomerSDKStatsMetrics }) => CustomerSDKStatsMetrics.getInstance({ | ||
| instrumentationKey: options.instrumentationKey, | ||
| endpointUrl: options.endpointUrl, | ||
| disableOfflineStorage: this.disableOfflineStorage, | ||
| networkCollectionInterval: exportInterval, | ||
| })) | ||
| .then((metrics) => { | ||
| this.customerSDKStatsMetrics = metrics; | ||
| }) | ||
| .catch((error) => { | ||
| diag.warn("Failed to initialize customer SDK stats metrics:", error); |
There was a problem hiding this comment.
The nested promise chain could be simplified using async/await for better readability and error handling. Consider wrapping this in an async function to make the code more maintainable.
Copilot uses AI. Check for mistakes.
Packages impacted by this PR
@azure/monitor-opentelemetry-exporter
Issues associated with this PR
#36066
Describe the problem that is addressed by this PR
This pull request refactors the initialization of the customer SDK stats metrics and the statsbeat exporter to resolve circular dependency issues and improve asynchronous handling. The main changes involve switching to dynamic imports and lazy initialization, updating the singleton pattern to be asynchronous, and adjusting related tests and usages to accommodate these changes.
Circular dependency resolution and async initialization:
CustomerSDKStatsMetricsto use an async singleton pattern with dynamic import ofAzureMonitorStatsbeatExporter, allowing for asynchronous and circular-dependency-safe initialization. (customerSDKStats.ts, [1] [2]BaseSenderto asynchronously import and initializeCustomerSDKStatsMetrics, ensuring it only initializes if not already set and logging warnings if initialization fails. (baseSender.ts, [1] [2]Statsbeat exporter improvements:
AzureMonitorStatsbeatExporterto lazily initialize the HTTP sender using dynamic import, storing sender options for later use, and updating export/shutdown logic to handle async sender initialization. (statsbeatExporter.ts, [1] [2] [3]Test updates for async changes:
getInstancemethod forCustomerSDKStatsMetrics, ensuring proper setup and teardown in test environments. (baseSender.spec.ts, [1];customerSDKStats.spec.ts, [2]Code cleanup:
anywhere necessary to accommodate dynamic imports and async initialization. (customerSDKStats.ts, [1] [2];statsbeatExporter.ts, [3];baseSender.ts, [4]These changes collectively improve the reliability and maintainability of statsbeat metrics and exporter initialization, especially in environments where circular dependencies previously caused issues.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Yes
Checklists