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

When invalid combination of clients are used return appropriate Result instead of panic #2690

Open
cijothomas opened this issue Feb 20, 2025 · 5 comments
Labels
A-common Area:common issues that not related to specific pillar enhancement New feature or request help wanted Good for taking. Extra help will be provided by maintainers/approvers

Comments

@cijothomas
Copy link
Member

Suggested here:
#2673 (comment)

It might be possible for the provider construction to check if an invalid pair of Processor+Exporter client is used and return an Error...As of now, wrong combinations are not detected until too late, where things panic.

@cijothomas
Copy link
Member Author

eg:
provider.with_batch_exporter(exporter-with_unsupported_client_like_request) should return a Result. The Err should clearly indicate the reason for failure.

@cijothomas cijothomas added enhancement New feature or request help wanted Good for taking. Extra help will be provided by maintainers/approvers A-common Area:common issues that not related to specific pillar labels Feb 20, 2025
@Shunpoco
Copy link
Contributor

Shunpoco commented Mar 6, 2025

I'm interested in this issue, but

provider.with_batch_exporter(exporter-with_unsupported_client_like_request) should return a Result.

Currently all with_xxx methods return Builder itself. Do you think we should change all methods (or at least with_batch_exporter's) signature to return Result?

@cijothomas
Copy link
Member Author

I was referring to the .build() method to detect invalid combination and return Result.

Related to this is #2654 which only does Exporter specific one. This issue is about the overall provider..Some detailed analysis is to be done to find a good balance.

@Shunpoco
Copy link
Contributor

Shunpoco commented Mar 6, 2025

Thanks! I will take a look at the PR and consider how to do it in providers.

Some detailed analysis is to be done to find a good balance

Do you already have any concerns to change build() to return Result?

@cijothomas
Copy link
Member Author

Do you already have any concerns to change build() to return Result?

No. I prefer build() returns Result. The part that needs design is what Error enum it would return. We probably need to create a new one, with the variants reflecting the possible ways it can fail. OR return string only as Error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar enhancement New feature or request help wanted Good for taking. Extra help will be provided by maintainers/approvers
Projects
None yet
Development

No branches or pull requests

2 participants