fix(core): avoid leaking Node.js types via unrefTimer() util #5986
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@ ## main #5986 +/- ## ========================================== + Coverage 95.23% 95.24% +0.01% ========================================== Files 315 315 Lines 8598 8602 +4 Branches 1791 1795 +4 ========================================== + Hits 8188 8193 +5 + Misses 410 409 -1
🚀 New features to boost your workflow:
|
| | ||
| private _finishedLogRecords: SdkLogRecord[] = []; | ||
| private _timer: NodeJS.Timeout | undefined; | ||
| private _timer: NodeJS.Timeout | number | undefined; |
There was a problem hiding this comment.
we are removing NodeJS.Timer type in src/common/timer-util.ts but this private property still holds NodeJS.Timeout. I've checked the type signature and saw that NodeJS.Timeout extends NodeJS.Timer. Should we replace the type here as well?
There was a problem hiding this comment.
I think it should be safe to keep. The private property's type gets erased in the final type definition. As a consequence, /// <reference types="node" /> does not end up in the BatchLogRecordProcessor.d.ts :)
import type { BufferConfig } from '../types'; import type { SdkLogRecord } from './SdkLogRecord'; import type { LogRecordExporter } from './LogRecordExporter'; import type { LogRecordProcessor } from '../LogRecordProcessor'; export declare abstract class BatchLogRecordProcessorBase<T extends BufferConfig> implements LogRecordProcessor { private readonly _exporter; private readonly _maxExportBatchSize; private readonly _maxQueueSize; private readonly _scheduledDelayMillis; private readonly _exportTimeoutMillis; private _finishedLogRecords; private _timer; private _shutdownOnce; constructor(_exporter: LogRecordExporter, config?: T); onEmit(logRecord: SdkLogRecord): void; forceFlush(): Promise<void>; shutdown(): Promise<void>; private _shutdown; /** Add a LogRecord in the buffer. */ private _addToBuffer; /** * Send all LogRecords to the exporter respecting the batch size limit * This function is used only on forceFlush or shutdown, * for all other cases _flush should be used * */ private _flushAll; private _flushOneBatch; private _maybeStartTimer; private _clearTimer; private _export; protected abstract onShutdown(): void; } //# sourceMappingURL=BatchLogRecordProcessorBase.d.ts.map
| * @deprecated please copy this code to your implementation instead, this function will be removed in the next major version of this package. | ||
| * @param timer | ||
| */ | ||
| export function unrefTimer(timer: { unref(): unknown } | number): void { |
There was a problem hiding this comment.
I'm wondering if it is worth to keep this and make the argument a Refable, like https://nodejs.org/api/process.html#processunrefmayberefable, so that we can reduce the repetitiveness.
legendecas left a comment
legendecas left a comment There was a problem hiding this comment.
Deprecating the util functions in the core package also makes sense
Thanks, I think I'll leave it like this then 🙂 By deprecating and eventually removing it we can eventually reduce the amount of public APIs that end-users don't need to use directly, so I think the duplication is worthwhile in the end. 🙂 |
3 participants
Which problem is this PR solving?
See #5916
This PR partially addresses #5916.
unrefTimer()does reference Node.js types and leaks them even if you're not working on a Node.js project. This PR fixes this instance by widening the type to accept{ unref(): unknown } | numberinstead of justNodeJS.Timer, getting rid of the platform specific implementations in the process.Since the utility function does not really do much, this PR also deprecates it. It also in-lines any existing usages so that we can easily remove it in the next major version.
This is pretty difficult to test since it does not fully solve the problem yet:
otperformancedoes still leak the Node.js types - I'll fix this in a follow-up.In the meantime, you can verify that this actually not referencing Node.js types anymore by following these steps:
npm cinpm run compilecd packagesgrep -r '/// <reference types="node"'You can then verify that
timer-util.d.tsis not listed in the output anymore:(Note: the occurrence in
@opentelemetry/resourcesshould not cause any issues, since it's not part of the public API,@opentelemetry/exporter-jaegeris not supported in the browser)