feat: implement transaction context #315
Conversation
federicobond
force-pushed
the
feat/transaction-context
branch
2 times, most recently
from
7f2da6b
to
b622d21
Compare
federicobond
force-pushed
the
feat/transaction-context
branch
from
b622d21
to
11d580d
Compare
federicobond
changed the title
| ] | ||
|
|
||
| _evaluation_context = EvaluationContext() | ||
|
|
||
| _hooks: typing.List[Hook] = [] | ||
|
|
||
| _transaction_context_propagator: TransactionContextPropagator = ( | ||
| NoopTransactionContextPropagator() |
There was a problem hiding this comment.
Should the SDK initialize a functional transaction context propagator by default?
There was a problem hiding this comment.
We do something similar to this in JS. What would the alternative be?
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| def get_transaction_context() -> EvaluationContext: | ||
| return _transaction_context_propagator.get_transaction_context() |
There was a problem hiding this comment.
Is this the right API or do we want something based on decorators/context managers?
There was a problem hiding this comment.
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.
Yes, the comment should have been in the set_transaction_context. I was wondering if we should do something like this instead:
with start_transaction_context(context):
continue_transaction() # context will be cleaned up after the block completesSimilar to the way it works in JS by taking a callback.
federicobond
force-pushed
the
feat/transaction-context
branch
from
11d580d
to
1c72782
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #315 +/- ##
==========================================
- Coverage 97.32% 97.18% -0.15%
==========================================
Files 26 28 +2
Lines 1198 1244 +46
==========================================
+ Hits 1166 1209 +43
- Misses 32 35 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Federico Bond <[email protected]>
federicobond
force-pushed
the
feat/transaction-context
branch
from
1c72782
to
c31a2fa
Compare
This is a draft implementation of transaction context propagation via contextvars.