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

Fix batch parameter requirements #4388

Closed

Conversation

jack-berg
Copy link
Member

Fixes a bug which has been present since the original introduction of standard environment variables in #666 and the original trace SDK spec #205.

@breedx-splk points out that there's no need for the export batch size to be less than the queue size since an export batch size smaller than the queue can still drain a queue in multiple exports.

On the other side of things, an export batch size large than the queue will always fully drain the queue with each export. And so you could say there's no reason for the export batch size to be larger than the max queue size, but its hardly worth restricting.

@reyang
Copy link
Member

reyang commented Jan 28, 2025

What's the benefit of this change? I think the current version can help to prevent user mistakes (e.g. if someone tries to change the batch size to 500, while the queue size is 100, SDKs have a chance to throw exception or warn the user so they can fix the problem ahead of time).

@chukunx
Copy link

chukunx commented Feb 2, 2025

Thanks @jack-berg for bring up the discussion and @reyang for chiming in. My two cents: the original restriction (max batch size <= max queue size) acts as a safeguard to catch potential misconfigurations and inform users when they are (or are about to be) running into unintended or undefined behavior. At the specification level, this helps enforce consistent behavior across languages.

However, each language may introduce implementations that provide additional flexibility or functionality based on language features and user needs, etc. Such implementations may be beyond the scope of the specification IMHO.

As a reference on this matter, I took a quick look at some implementations (I’m not familiar with each language, so SMEs, please feel free to correct me if needed):

It does look like these languages enforce the current restriction.

@jack-berg
Copy link
Member Author

Thanks for that @chukunx! To @reyang's point, I don't know of any use cases that are enabled by a looser restriction here. Without a use case in mind and with all the implementations (except java which I consider a bug) producing an error or effectively ignoring this configuration as invalid, I don't see any reason to loosen the requirement.

@jack-berg jack-berg closed this Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants