perf(opentelemetry): Bucket spans for cleanup #14154
Conversation
lforst
changed the title
| } | ||
|
|
||
| /** Try to flush any pending spans immediately. */ | ||
| public flush(): void { | ||
| this._clearTimeout(); | ||
|
|
||
| const openSpanCount = this._finishedSpans.length; | ||
| const finishedSpans: ReadableSpan[] = []; | ||
| this._finishedSpanBuckets.forEach(bucket => { |
There was a problem hiding this comment.
l: Can we instead add a get _finishSpans(): ReadableSpan[] getter on the exporter class and use this? This makes it easier to test this as well as we can just access this in tests too? 🤔
There was a problem hiding this comment.
I will not add a public function for this. This is an implementation detail that should not be exposed. I can however extract this functionality into a private fn, which I also do not particularly like since we do this operation exactly one time (apart from in tests, which is whatever).
There was a problem hiding this comment.
yeah I do not mean a public function, but just making it a private method on the class, then it is also not exposed but we can use it in tests. right now we duplicate the logic in the test so if that runs apart we may not notice (it's really a nit so feel free to disregard). In the tests we could then do, instead of:
const finishedSpans1 = [];
exporter['_finishedSpanBuckets'].forEach((bucket: any) => {
if (bucket) {
finishedSpans1.push(...bucket.spans);
}
});just
const finishedSpans1 = exporter['_finishedSpans'];or something like this.
| public constructor(options?: { timeout?: number }) { | ||
| this._finishedSpans = []; | ||
| this._timeout = options?.timeout || DEFAULT_TIMEOUT; | ||
| private _finishedSpanBuckets: (FinishedSpanBucket | undefined)[]; |
There was a problem hiding this comment.
l: Could we add comments to these explaining what they do here, e.g. why do we keep the spansToBucketEntry etc? it all makes sense to me, but for the sake of our future selves :D
yarn lint) & (yarn test).We had a huge performance problem with a overhead of 5x or so in CPU Load. We investigated the issue and saw that the cleanup logic for OTEL ran on every span end. Thats why we rewrote this to run only once every interval.
I do not guarantee that this code is perfect, this is just a hint, that there is something wrong currently.