Skip to content
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

Batching Span Processor maxQueueSize is meaningless #3512

Closed
dyladan opened this issue May 17, 2023 · 2 comments
Closed

Batching Span Processor maxQueueSize is meaningless #3512

dyladan opened this issue May 17, 2023 · 2 comments
Assignees
Labels
area:sdk Related to the SDK spec:trace Related to the specification/trace directory

Comments

@dyladan
Copy link
Member

dyladan commented May 17, 2023

I was looking through a PR to the JS batch span processor and noticed that with the current spec wording, it is impossible to fill the queue beyond maxExportBatchSize because if maxExportBatchSize spans are in the queue an export is triggered. During the review of #3024 it was made clear that multiple batches can be exported concurrently (#3024 (comment), see also #2452).

If a batching span processor exports a batch every time maxExportBatchSize are in the queue, and there is no limit to the number of concurrent batches, then the queue can never have more than maxExportBatchSize spans.

@dyladan dyladan added the spec:trace Related to the specification/trace directory label May 17, 2023
@arminru arminru added the area:sdk Related to the SDK label May 23, 2023
@arminru
Copy link
Member

arminru commented May 23, 2023

@dyladan Have you checked how other SDK implementations handle this?

If spans are not removed from the queue immediately when an export is triggered by reaching maxExportBatchSize, they would still contribute to the usage of the queue, right?

@dyladan
Copy link
Member Author

dyladan commented Jun 14, 2023

Picking this back up as it came back up in the SIG meeting again today. Looks like g: Java, Ruby, Swift, Python all eagerly export batches but because they're threaded they have slightly different concerns/semantics than JS. They all appear to have a single worker thread that does exporting in a blocking fashion so it is possible for the queue to fill while an export is happening. Because of this, they only have a single export happening at a given time.

Originally I thought limiting to only 1 export at a time was a spec requirement, but this thread shows that it is not. Despite the fact it is not a spec requirement, most SDKs appear to have that behavior. The max queue size doesn't make sense in JS because we don't limit export concurrency. Right now without eager exporting we can have up to exportTimeout / exportInterval concurrent exports. If we add eager exporting, we will have unbounded concurrency (I expect this is not wanted).

We want to have eager exporting so I expect we will have to implement some concurrency restriction (despite the fact that it is not required by spec).


The batch span processor spec is a mess and I think now that I only made the situation worse in #3024. I'm not sure how any new SDK that implemented the spec today would interpret it without just mimicking what another SDK does. I think that the original statement of maxQueueSize being meaningless isn't exactly correct anymore, but only because existing BSP implementations are implemented in such a way that it is possible to fill the queue. An unbounded concurrency version like what JS would have if we implemented eager exporting doesn't violate the spec, but I think it should.

For a time I thought I would try to clean up the BSP spec, but I think I just don't have the cycles to do it. Pouring one out for whoever has to interpret or modify this section of the spec next after me.

@dyladan dyladan closed this as completed Aug 11, 2023
@dyladan dyladan closed this as not planned Won't fix, can't repro, duplicate, stale Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:sdk Related to the SDK spec:trace Related to the specification/trace directory
Projects
None yet
Development

No branches or pull requests

3 participants