perf(opentelemetry): Bucket spans for cleanup#14154
perf(opentelemetry): Bucket spans for cleanup#14154lforst merged 9 commits intogetsentry:developfrom
Conversation
lforst
changed the title
|
|
||
| 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.
|
@mydea I'd like to merge this sooner rather than later. |
|
This code breaks if you configure
|
|
Huh, I do not think that setting this to |
|
Purely semantically I think it makes sense to support "wait an infinite time for spans to complete". A workaround for now is to set it to a sufficiently large number (don't have enough memory for |
|
Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up! |
Today, if you pass e.g. `Infinity` or another large number to `maxSpanWaitDuration`, the SDK will error out because we try to do `new Array(maxSpanWaitDuration)`, which is impossible. Ideally, we can properly allow an infinite wait time, but for now this adds checks to ensure only valid values are passed in there, and we do not explode. See #14154 (comment)
5 participants
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.