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

Validate maxExportBatchSize in BatchSpanProcessor #3155

Closed
morigs opened this issue Aug 10, 2022 · 1 comment · Fixed by #3232
Closed

Validate maxExportBatchSize in BatchSpanProcessor #3155

morigs opened this issue Aug 10, 2022 · 1 comment · Fixed by #3232
Assignees
Labels
feature-request sdk:traces Issues and PRs related to the Traces SDK

Comments

@morigs
Copy link
Contributor

morigs commented Aug 10, 2022

Is your feature request related to a problem? Please describe.

Some exporters README's mention that the maxExportBatchSize setting of the BatchSpanProcessor must be maller or equal to maxQueueSize. But no validation is performed in code.
It's difficult to track the correctness of the values by hand, because one of the values can be modified and another use default...

Describe the solution you'd like

Perhaps the safest solution would be to write warning to logs and use default values if supplied settings are invalid

Describe alternatives you've considered

Another solution is to throw an error from the constructor. But this is a breaking change and unsafe

Additional context

We tried to add validation in application logic, but it's impossible, because we can't know default values for sure.

@legendecas legendecas added the sdk:traces Issues and PRs related to the Traces SDK label Sep 2, 2022
@samimusallam
Copy link
Member

Can this issue be assigned to me? I would go with the first suggested solution, to use defaults if we get invalid settings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request sdk:traces Issues and PRs related to the Traces SDK
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants