open-feature / open-feature/go-sdk

Deprecate MergeTransactionContext function

Open
#455 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
249
Forks
61
Avg merge
2d 10h
Merged PRs (30d)
20

Description

Related to #454

[MergeTransactionContext](https://github.com/open-feature/go-sdk/blob/v1.17.0/openfeature/evaluation_context.go#L75) does a couple of things:
- Call `TransactionContext` to extract an EvaluationContext from the given context.Context
- Merge the given EvaluationContext with the one extracted from the step above
- Return a new context.Context that contains the merged EvaluationContext from the step above.

I propose that we deprecate this function. I would argue that it's doing too much and naming it more appropriately is not possible without creating a really long name.

Before we attempt to fill the gap, I'd like to establish the usefulness of this function, given that it's not defined in the spec (https://openfeature.dev/specification/sections/evaluation-context). Here I am specifically referring to the ability of merging evaluation contexts, whether directly via a function that takes multiple evaluation contexts, or indirectly by merging the "transaction context" in the "transaction context propagator" with a different evaluation context. We need to determine whether similar behaviour exists in the other languages. If so, we should consider updating the spec if we want this functionality implemented consistently across all languages.

Either way, there are two ways to implement this functionality, if we decide that we need this:

1. Export (and rename) the existing `mergeContexts` function to allow users to merge evaluation contexts.
2. Merge evaluation contexts indirectly as described in the paragraph above (essentially what MergeTransactionContext does), but rename the function. We'll need to come up with a good name in this case.

https://github.com/open-feature/go-sdk/blob/4b3d6d953076f3056d78c512b31c9fc2ec29ab89/openfeature/client.go#L836

I personally prefer the first option, which is a more more modular approach. I think that the existing MergeTransactionContext function does too many things.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.