-
Notifications
You must be signed in to change notification settings - Fork 35
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
chore: lazy load internal request module #519
chore: lazy load internal request module #519
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
- Coverage 81.95% 81.90% -0.06%
==========================================
Files 54 54
Lines 2206 2205 -1
Branches 512 513 +1
==========================================
- Hits 1808 1806 -2
+ Misses 334 333 -1
- Partials 64 66 +2 ☔ View full report in Codecov by Sentry. |
…github.com/DataDog/datadog-lambda-js into jordan.gonzalez/lazy-load-fallback-processor
@@ -56,7 +54,7 @@ export interface MetricsConfig { | |||
} | |||
|
|||
export class MetricsListener { | |||
private currentProcessor?: Promise<Processor>; | |||
private currentProcessor?: Promise<any>; |
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.
Sadly, this has to stay as it, so we don't import and load the modules that add 1MB of memory allocation.
@@ -33,6 +33,8 @@ export function unpatchHttp() { | |||
} | |||
|
|||
function patchMethod(mod: typeof http | typeof https, method: "get" | "request", contextService: TraceContextService) { | |||
if (mod[method].__wrapped !== undefined) return; // Only patch once |
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.
This was missed, required so no repeated logs appear.
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.
This is the best thing since sliced bread.
const processor = new Processor(apiClient, metricsBatchSendIntervalMS, config.shouldRetryMetrics); | ||
processor.startProcessing(); | ||
return processor; | ||
if (!this.isExtensionRunning && !this.config.logForwarding) { |
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.
nice!
What does this PR do?
Reduces memory allocation for this package by lazy loading internal
request.ts
methods.Before:
After:
Motivation
It was possible to reduce the memory allocations + performance.
Testing Guidelines
Additional Notes
SVLS-4628
Allocation time can vary, but now we are sure that the lazy load is in effect.
Types of Changes
Check all that apply