open-policy-agent/opa

Allow returning errors in HTTPTracingService

Opened this issue · 6 comments

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.

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?

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)

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?

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

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

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

stale commented

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.