perf(opentelemetry): Bucket spans for cleanup

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!

[fmorett]

• If you've added code that should be tested, please add tests.
• Ensure your code lints and the test suite passes (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.

[AbhiPrasad]

Hey @fmorett thank you for the PR.

I opened a GH issue in #14177 for this so that we can discuss further as a group and see what other users think as well.

We'll try to get a maintainer assigned to this ASAP - appreciate your patience in the meantime.

[mydea]

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? 🤔

[lforst]

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).

[mydea]

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:

constfinishedSpans1=[];exporter['_finishedSpanBuckets'].forEach((bucket: any)=>{if(bucket){finishedSpans1.push(...bucket.spans);}});

just

constfinishedSpans1=exporter['_finishedSpans'];

or something like this.

[mydea]

@fmorett any chance you could test @lforst 's updated implementation in this PR and see if that actually fixes/improves the overhead? FWIW I think it is a good change regardless, but would be cool to know if this actually fixes the problem or if another problem remains.

[lforst]

@mydea I'd like to merge this sooner rather than later.

[mydea]

[jeengbe]

This code breaks if you configure maxSpanWaitDuration: Infinity 🙂 I sadly don't have the time to fix it myself

[mydea]

Huh, I do not think that setting this to Infinity is really a supported use case right now, but we can probably be more explicit about this and validate this against a max. size - I'll open a PR for this.

[jeengbe]

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 Number.MAX_SAFE_INTEGER though)

[mydea]

Yeah, I agree. We will probably/hopefully revisit this in the future anyhow, for now we can at least avoid this blowing up!