feat: Implement transaction context #312

askpt · 2024-11-11T10:42:48Z

Transaction Context

This pull request introduces transaction context propagation to the OpenFeature library. The changes include adding a new interface for transaction context propagation, implementing a no-op and an AsyncLocal-based propagator, and updating the API to support these propagators.

Related Issues

Fixes #243

Notes

Transaction Context Propagation:

src/OpenFeature/Api.cs: Added a private field _transactionContextPropagator and a lock for thread safety. Introduced methods to get and set the transaction context propagator, and to manage the transaction context using the propagator. [1] [2]
src/OpenFeature/AsyncLocalTransactionContextPropagator.cs: Implemented the AsyncLocalTransactionContextPropagator class that uses AsyncLocal<T> to store the transaction context.
src/OpenFeature/NoOpTransactionContextPropagator.cs: Implemented a no-op version of the ITransactionContextPropagator interface.
src/OpenFeature/Model/ITransactionContextPropagator.cs: Defined the ITransactionContextPropagator interface responsible for persisting transaction contexts.

API Enhancements:

src/OpenFeature/Api.cs: Updated the ShutdownAsync method to reset the transaction context propagator.
src/OpenFeature/OpenFeatureClient.cs: Modified the EvaluateFlagAsync method to merge the transaction context with the evaluation context.

codecov · 2024-11-15T08:19:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.53%. Comparing base (2774b0d) to head (59ddca7).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #312      +/-   ##
==========================================
+ Coverage   85.17%   85.53%   +0.36%     
==========================================
  Files          36       38       +2     
  Lines        1477     1514      +37     
  Branches      150      153       +3     
==========================================
+ Hits         1258     1295      +37     
  Misses        188      188              
  Partials       31       31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lukas-reining

Nice start! Left some thoughts and references to how we are doing it in the other languages.

src/OpenFeature/Model/TransactionContext.cs

src/OpenFeature/Model/ContextBuilder.cs

src/OpenFeature/Api.cs

askpt · 2024-11-26T18:40:08Z

Nice start! Left some thoughts and references to how we are doing it in the other languages.

Thanks for the review @lukas-reining. I based the implementation on the JS client. I'll look into more detail on the Java version instead.

Signed-off-by: André Silva <[email protected]>

toddbaert · 2024-12-31T13:18:01Z

src/OpenFeature/Api.cs

+        /// Return the transaction context propagator.
+        /// </summary>
+        /// <returns><see cref="ITransactionContextPropagator"/>the registered transaction context propagator</returns>
+        public ITransactionContextPropagator GetTransactionContextPropagator()


I don't think we need this accessor, if we need it for tests or internally that's fine, but let's make it internal in that case.

I followed the java-sdk implementation. I am happy to change it to internal if it is the expected behaviour.

Let me know if I should remove or mark it as internal.

Ah, I didn't realize we implemented it there. I'm not of the opinion it's a great idea, but since it's also there it's not a blocker for me (it's not in the spec).

I can go either way... do you think we should have it?

Approving now.

Does anyone outside of the client need to access the Transaction Context Propagator? Likely not...

I prefer to keep it internal so we expose what is necessary. But I don't like different language implementations going in different directions, and I don't know what the best approach is.

I prefer to keep it internal so we expose what is necessary.

I agree.

But I don't like different language implementations going in different directions, and I don't know what the best approach is.

Of course I generally agree, that's why we have a spec, but there's always going to be some deviations and extras. If anything, we should probably deprecate the Java function and remove it eventually. I don't see it in JS either.

Changed visibility here: bf4dbb4

Signed-off-by: Todd Baert <[email protected]>

toddbaert · 2024-12-31T13:45:10Z

This looks right to me, though unless there's a good reason, I think we should remove this. Besides that I'm ready to approve.

I also pushed a small change to add docs to the README. @askpt please check them and make sure they make sense to you (it's a similar example to other SDKs).

toddbaert

Approving, but highlighting this thread: #312 (comment)

Signed-off-by: André Silva <[email protected]>

chrfwow · 2025-01-02T09:26:37Z

src/OpenFeature/AsyncLocalTransactionContextPropagator.cs

+namespace OpenFeature;
+
+/// <summary>
+///  It is a no-op implementation of <see cref="ITransactionContextPropagator"/>


I would not consider this a No-Op implementation. Maybe this line should be moved to the NoOpTransactionContextPropagator?

Thanks! Fixed with: 7a60997

chrfwow · 2025-01-02T09:29:57Z

src/OpenFeature/OpenFeatureClient.cs

@@ -221,6 +221,7 @@ private async Task<FlagEvaluationDetails<T>> EvaluateFlagAsync<T>(
            evaluationContextBuilder.Merge(evaluationContext);
            evaluationContextBuilder.Merge(this.GetContext());
            evaluationContextBuilder.Merge(context);
+            evaluationContextBuilder.Merge(Api.Instance.GetTransactionContext());


According to Requirement 3.2.3, the order should be API (global; lowest precedence) - transaction - client - invocation - before hooks (highest precedence), so I believe evaluationContextBuilder.Merge(context); should be the last merge call.
I think we should alos have a test to verify the correct merge order.

Good catch! Updated: 59ddca7

@toddbaert please review this part again 👍

chrfwow · 2025-01-02T09:31:50Z

test/OpenFeature.Tests/AsyncLocalTransactionContextPropagatorTests.cs

+    {
+        // Arrange
+        var propagator = new AsyncLocalTransactionContextPropagator();
+        var evaluationContext = EvaluationContext.Empty;


Maybe we should use a different value here, so that we can be sure that we actually get the value we stored in the propagator, and not just the default empty value

Changed: 1769be8

chrfwow · 2025-01-02T09:35:20Z

test/OpenFeature.Tests/OpenFeatureTests.cs

+        {
+            // Arrange
+            var api = Api.Instance;
+            var evaluationContext = EvaluationContext.Empty;


Same as previous comment

Changed: 1769be8

chrfwow · 2025-01-02T09:44:37Z

README.md

+
+```csharp
+// registering the AsyncLocalTransactionContextPropagator
+Api.Instance.SetTransactionContextPropagator(new AsyncLocalTransactionContextPropagator());


Out of curiosity, why don't we set a AsyncLocalTransactionContextPropagator as default value? Then the user would not have to set one manually first, which would remove one potential error source.

I followed the Java SDK implementation on this: https://github.com/open-feature/java-sdk/blob/f1817d8fef585f957de1cfb9222b03cb591ed2e9/src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java#L31

Signed-off-by: André Silva <[email protected]>

chrfwow

Looks good

askpt linked an issue Nov 11, 2024 that may be closed by this pull request

[FEATURE] Implement transaction context #243

Open

toddbaert requested review from benjiro and lukas-reining November 26, 2024 15:22

lukas-reining reviewed Nov 26, 2024

View reviewed changes

src/OpenFeature/Model/TransactionContext.cs Outdated Show resolved Hide resolved

src/OpenFeature/Model/ContextBuilder.cs Outdated Show resolved Hide resolved

src/OpenFeature/Api.cs Outdated Show resolved Hide resolved

src/OpenFeature/Api.cs Outdated Show resolved Hide resolved

askpt force-pushed the askpt/243-feature-implement-transaction-context branch from 35b90bf to 9158a92 Compare December 4, 2024 17:05

askpt closed this Dec 10, 2024

askpt force-pushed the askpt/243-feature-implement-transaction-context branch from a0a7f36 to c471c06 Compare December 10, 2024 08:34

Changing API client and added Context Propagator.

264d8c9

Signed-off-by: André Silva <[email protected]>

askpt reopened this Dec 10, 2024

askpt added 16 commits December 12, 2024 08:18

Merge branch 'main' into askpt/243-feature-implement-transaction-context

c15192a

Merge branch 'main' into askpt/243-feature-implement-transaction-context

3539674

Merge branch 'main' into askpt/243-feature-implement-transaction-context

63c3f62

Adding NoOpTransactionContextPropagator.

6a870ff

Signed-off-by: André Silva <[email protected]>

Making field readonly.

8bcfc99

Signed-off-by: André Silva <[email protected]>

Added an empty context return.

50613fe

Signed-off-by: André Silva <[email protected]>

Adding unit tests.

b67b659

Signed-off-by: André Silva <[email protected]>

Merge branch 'main' into askpt/243-feature-implement-transaction-context

b0a20d7

Fix unit test.

02f93dd

Signed-off-by: André Silva <[email protected]>

Adding clear transaction context to shutdown.

8d45264

Signed-off-by: André Silva <[email protected]>

Merge branch 'main' into askpt/243-feature-implement-transaction-context

8e3789b

Merging transaction context at evaluation.

d9ed730

Signed-off-by: André Silva <[email protected]>

Adding an AsyncLocalTransactionContextPropagator.

9f75a44

Signed-off-by: André Silva <[email protected]>

Merge branch 'main' into askpt/243-feature-implement-transaction-context

e473ccc

Adding unit tests.

4146f0e

Signed-off-by: André Silva <[email protected]>

Adding missing tests.

64f5c68

Signed-off-by: André Silva <[email protected]>

askpt requested a review from lukas-reining December 20, 2024 08:52

askpt marked this pull request as ready for review December 20, 2024 08:52

askpt requested a review from a team as a code owner December 20, 2024 08:52

toddbaert reviewed Dec 31, 2024

View reviewed changes

fixup: add docs

f461152

Signed-off-by: Todd Baert <[email protected]>

toddbaert requested review from arttonoyan, kinyoklion, beeme1mr and chrfwow December 31, 2024 13:45

toddbaert mentioned this pull request Dec 31, 2024

chore(main): release 2.3.0 #330

Open

toddbaert self-requested a review December 31, 2024 14:15

toddbaert approved these changes Dec 31, 2024

View reviewed changes

askpt added 2 commits December 31, 2024 14:32

Change visibility of GetPropagator.

bf4dbb4

Signed-off-by: André Silva <[email protected]>

Merge branch 'main' into askpt/243-feature-implement-transaction-context

e480050

chrfwow reviewed Jan 2, 2025

View reviewed changes

askpt added 3 commits January 2, 2025 14:15

Fixed comment.

7a60997

Signed-off-by: André Silva <[email protected]>

Replaced the context.

1769be8

Signed-off-by: André Silva <[email protected]>

Fixing the context merging order. Updated unit test.

59ddca7

Signed-off-by: André Silva <[email protected]>

askpt requested a review from chrfwow January 2, 2025 15:22

chrfwow approved these changes Jan 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement transaction context #312

feat: Implement transaction context #312

askpt commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

lukas-reining left a comment

askpt commented Nov 26, 2024

toddbaert Dec 31, 2024 •

edited

Loading

askpt Dec 31, 2024

toddbaert Dec 31, 2024

askpt Dec 31, 2024

toddbaert Dec 31, 2024

askpt Dec 31, 2024 •

edited

Loading

toddbaert commented Dec 31, 2024 •

edited

Loading

toddbaert left a comment

chrfwow Jan 2, 2025

askpt Jan 2, 2025

chrfwow Jan 2, 2025

askpt Jan 2, 2025

chrfwow Jan 2, 2025

askpt Jan 2, 2025

chrfwow Jan 2, 2025

askpt Jan 2, 2025

chrfwow Jan 2, 2025

askpt Jan 2, 2025

chrfwow left a comment

feat: Implement transaction context #312

Are you sure you want to change the base?

feat: Implement transaction context #312

Conversation

askpt commented Nov 11, 2024 • edited Loading

Transaction Context

Related Issues

Notes

Transaction Context Propagation:

API Enhancements:

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

lukas-reining left a comment

Choose a reason for hiding this comment

askpt commented Nov 26, 2024

toddbaert Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askpt Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

toddbaert commented Dec 31, 2024 • edited Loading

toddbaert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrfwow left a comment

Choose a reason for hiding this comment

askpt commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

toddbaert Dec 31, 2024 •

edited

Loading

askpt Dec 31, 2024 •

edited

Loading

toddbaert commented Dec 31, 2024 •

edited

Loading