-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: implement transaction context #315
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,10 @@ | |
from openfeature.provider import FeatureProvider | ||
from openfeature.provider._registry import provider_registry | ||
from openfeature.provider.metadata import Metadata | ||
from openfeature.transaction_context import ( | ||
NoopTransactionContextPropagator, | ||
TransactionContextPropagator, | ||
) | ||
|
||
__all__ = [ | ||
"get_client", | ||
|
@@ -26,12 +30,19 @@ | |
"shutdown", | ||
"add_handler", | ||
"remove_handler", | ||
"set_transaction_context_propagator", | ||
"set_transaction_context", | ||
"get_transaction_context", | ||
] | ||
|
||
_evaluation_context = EvaluationContext() | ||
|
||
_hooks: typing.List[Hook] = [] | ||
|
||
_transaction_context_propagator: TransactionContextPropagator = ( | ||
NoopTransactionContextPropagator() | ||
) | ||
|
||
|
||
def get_client( | ||
domain: typing.Optional[str] = None, version: typing.Optional[str] = None | ||
|
@@ -94,3 +105,17 @@ | |
|
||
def remove_handler(event: ProviderEvent, handler: EventHandler) -> None: | ||
_event_support.remove_global_handler(event, handler) | ||
|
||
|
||
def set_transaction_context_propagator( | ||
propagator: TransactionContextPropagator, | ||
) -> None: | ||
_transaction_context_propagator = propagator | ||
|
||
|
||
def set_transaction_context(context: EvaluationContext) -> None: | ||
_transaction_context_propagator.set_transaction_context(context) | ||
|
||
|
||
def get_transaction_context() -> EvaluationContext: | ||
return _transaction_context_propagator.get_transaction_context() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this the right API or do we want something based on decorators/context managers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you elaborate on this? The main use case for accessing transaction context would be within the SDK when it merges context prior to flag evaluation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the comment should have been in the with start_transaction_context(context):
continue_transaction() # context will be cleaned up after the block completes Similar to the way it works in JS by taking a callback. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import typing | ||
from contextvars import ContextVar | ||
|
||
from openfeature.evaluation_context import EvaluationContext | ||
|
||
__all__ = [ | ||
"TransactionContextPropagator", | ||
"NoopTransactionContextPropagator", | ||
"ContextVarTransactionContextPropagator", | ||
] | ||
|
||
|
||
class TransactionContextPropagator(typing.Protocol): | ||
def get_transaction_context(self) -> EvaluationContext: ... | ||
|
||
def set_transaction_context(self, context: EvaluationContext) -> None: ... | ||
|
||
|
||
class NoopTransactionContextPropagator(TransactionContextPropagator): | ||
def get_transaction_context(self) -> EvaluationContext: | ||
return EvaluationContext() | ||
|
||
def set_transaction_context(self, context: EvaluationContext) -> None: | ||
pass | ||
|
||
|
||
class ContextVarTransactionContextPropagator(TransactionContextPropagator): | ||
def __init__(self) -> None: | ||
self._contextvar = ContextVar( | ||
"transaction_context", default=EvaluationContext() | ||
) | ||
|
||
def get_transaction_context(self) -> EvaluationContext: | ||
return self._contextvar.get() | ||
|
||
def set_transaction_context(self, context: EvaluationContext) -> None: | ||
self._contextvar.set(context) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
from concurrent.futures import ThreadPoolExecutor | ||
|
||
from openfeature.evaluation_context import EvaluationContext | ||
from openfeature.transaction_context import ContextVarTransactionContextPropagator | ||
|
||
|
||
def test_contextvar_transaction_context_propagator(): | ||
propagator = ContextVarTransactionContextPropagator() | ||
|
||
context = propagator.get_transaction_context() | ||
assert isinstance(context, EvaluationContext) | ||
|
||
context = EvaluationContext(targeting_key="foo", attributes={"key": "value"}) | ||
propagator.set_transaction_context(context) | ||
transaction_context = propagator.get_transaction_context() | ||
|
||
assert transaction_context.targeting_key == "foo" | ||
assert transaction_context.attributes == {"key": "value"} | ||
|
||
def thread_fn(): | ||
thread_context = propagator.get_transaction_context() | ||
assert thread_context.targeting_key is None | ||
assert thread_context.attributes == {} | ||
|
||
with ThreadPoolExecutor() as executor: | ||
future = executor.submit(thread_fn) | ||
|
||
future.result() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the SDK initialize a functional transaction context propagator by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do something similar to this in JS. What would the alternative be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a default implementation that is not a no-op. But it could introduce a performance penalty if you are not using it, so probably better to default to no-op. We might want to put out a warning if you call
set_transaction_context
with a NoopTransactionContextPropagator though.