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

Allow returning errors in HTTPTracingService #6710

Open
mjungsbluth opened this issue Apr 23, 2024 · 5 comments
Open

Allow returning errors in HTTPTracingService #6710

mjungsbluth opened this issue Apr 23, 2024 · 5 comments

Comments

@mjungsbluth
Copy link
Contributor

What is the underlying problem you're trying to solve?

When implementing a custom HTTPTracingService, the interface allows to create Transports or Handlers without the ability to return errors. This effectively leaves two options: panicking or disabling the tracing functionality with just a log line that hints at something that is broken. As the tracing options are passed as an untyped array, errors in construction of these options can happen.

Describe the ideal solution

Change the signature of the methods in HTTPTracingService to be able to return an error and cascade the problem up. Note that this is a breaking change for library users (albeit easy to fix)

Describe a "Good Enough" solution

Not really like it but a backwards compatible change would be introducing a second interface that can validate tracing.Options and is called before they are used with the tracing service.

@ashutosh-narkar
Copy link
Member

The ability to return an error seems like a good idea.

a backwards compatible change would be introducing a second interface that can validate tracing.Options

Are you suggesting a new Validate method on the Options type? This would also work, correct?

@mjungsbluth
Copy link
Contributor Author

Are you suggesting a new Validate method on the Options type? This would also work, correct?

That is also possible but is also a breaking change as far as I can see it. I was more thinking of another interface with a validate method that implementations HTTPTracingService can implement.

I am actually not sure how much concern just adding error as return type is as this interface is pretty specific to library use and also easy to fix (just return nil as err in the changed method signature)

@ashutosh-narkar
Copy link
Member

I am actually not sure how much concern just adding error as return type is as this interface is pretty specific to library use and also easy to fix (just return nil as err in the changed method signature)

You're probably correct but we don't know the impact of the breaking change. So another interface that returns an error and maybe takes a context as input if it makes sense would be better. We can deprecate the old one.

As the tracing options are passed as an untyped array, errors in construction of these options can happen.

Question: Where does the validation happen? Inside the NewTransport implementation?

@mjungsbluth
Copy link
Contributor Author

Question: Where does the validation happen? Inside the NewTransport implementation?

In my case yes. Causes can be type mismatches or missing elements.

@ashutosh-narkar
Copy link
Member

Ok. So a new interface that returns an error and deprecating the old seems like a good option to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants