open-feature / go-sdk

Go SDK for OpenFeature
https://openfeature.dev
Apache License 2.0
123 stars 29 forks source link

Add `context.Context` as first parameters to client/provider calls #72

Closed markphelps closed 1 year ago

markphelps commented 1 year ago

Apologies if this has already been discussed.

I may be missing something, but is it possible to add Go's context.Context as a first parameter to all client/provider methods (interface definitions)?

Ex: https://github.com/open-feature/go-sdk/blob/d1d0b5e95eaf8f7e8fff5d0b0a59e8af12c75dc2/pkg/openfeature/client.go#L16

would become something like:

BooleanValue(ctx context.Context, flag string, defaultValue bool, evalCtx EvaluationContext, options EvaluationOptions) (bool, error)

It seems as a user of this SDK that I would want to be able to use context to pass request timeouts upstream to the provider (ie: in case request to provider is taking too long to return, error which would return the 'default' value).

Is this meant to be done via a Hook per request? If so would it be possible to document this usage?

Although it still seems that passing context.Context as a first class (first) parameter would be ideal IMO since it is a Go convention.

skyerus commented 1 year ago

I agree that this would be beneficial and is definitely the idiomatic approach, I'm all for making this change.

As this would be another breaking change, we ought to decide and commit sooner rather than later.

cc @james-milligan @beeme1mr @toddbaert

james-milligan commented 1 year ago

i agree with @skyerus, context would be a great addition to the sdk

markphelps commented 1 year ago

Awesome. I'd be happy to create a PR, if nothing else to at least show what it would look like

skyerus commented 1 year ago

I've just created one @markphelps. If you get a chance please check if it's inline with what you had in mind

markphelps commented 1 year ago

Amazing, thanks for the quick turn around!

beeme1mr commented 1 year ago

Hey @markphelps, thanks again for your suggestion. It will be included in an upcoming release.