Skip to content
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

Specify the minimum set of contexts to be propagated #193

Open
Emily-Jiang opened this issue Jun 30, 2020 · 7 comments
Open

Specify the minimum set of contexts to be propagated #193

Emily-Jiang opened this issue Jun 30, 2020 · 7 comments

Comments

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Jun 30, 2020

In the current Context Propagation, by default it is unspecified what contexts (if any) are propagated by default. This is problematic when other specifications come to integrate with Context Propagation. I think it is useful for this spec to spec a minimum set of contexts to be propagated. The expected set will be in Security context, Application context, CDI context. Runtime can add what they also want to propagate over and above the minimum set.

@andymc12
Copy link

I agree that this (or a defined default set of contexts to propagate) is necessary for portability.

On today's MP Fault Tolerance call (which included several folks from this group), we discussed adding language to the FT and MP Rest Client specs that say that in environments where MP Context Propagation is available, that asynchronous operations should delegate to MP Context Propagation. My objection to this is that without a default set of contexts to propagate, users get no value when it comes to portability, but vendors must still cope with the existence or non-existence of MP CP - double the work, for no benefit.

I won't speak for the FT folks, but in Rest Client, users can specify their own ExecutorService to use for asynchronous operations. So, the best practice for portability would be to specify an executor that propagates the contexts that they want. They should be able to achieve this easier when using the MP CP APIs, but could also achieve it using other APIs. But if they don't specify their own executor, the behavior will be different when they move from Open Liberty to Quarkus, or from Payara to TomEE, etc. - and if the behavior is going to be different in this case, I don't see much benefit from adding language to the Rest Client spec that implementations should delegate to MP CP. This would allow implementations to decide how (or if) they will propagate contexts. IMO, the only reason to add this language to the Rest Client spec would be if the MP CP spec specified a default set (or minimum default set) of contexts to propagate - then that would benefit Rest Client users with added out-of-the-box portability.

@FroMage
Copy link
Contributor

FroMage commented Jul 1, 2020

I won't speak for the FT folks, but in Rest Client, users can specify their own ExecutorService to use for asynchronous operations. So, the best practice for portability would be to specify an executor that propagates the contexts that they want. They should be able to achieve this easier when using the MP CP APIs, but could also achieve it using other APIs. But if they don't specify their own executor, the behavior will be different when they move from Open Liberty to Quarkus, or from Payara to TomEE, etc. - and if the behavior is going to be different in this case, I don't see much benefit from adding language to the Rest Client spec that implementations should delegate to MP CP. This would allow implementations to decide how (or if) they will propagate contexts. IMO, the only reason to add this language to the Rest Client spec would be if the MP CP spec specified a default set (or minimum default set) of contexts to propagate - then that would benefit Rest Client users with added out-of-the-box portability.

This sounds like you're almost asking for the contexts to be propagated even if MP-CP is not involved, to let implementations pick whatever way they want to achieve that goal.

IMO the goal of the integration (roughly "if MP-CP is available, then it should be used to propagate contexts to the underlying threads, if any are created") is to make it clear that 1/ context propagation will happen just like everywhere else where MP-CP is used, in a coherent manner, and 2/ it will be configurable via MP-CP.

That's already a lot of value.

@FroMage
Copy link
Contributor

FroMage commented Jul 1, 2020

WRT the minimal set of contexts, it is still our opinion that transactions are essential and should not be left out of the minimum set of propagated contexts. Indeed this has been validated time and again but all our user feedback, who intuitively expect that the entirety of the request contexts are propagated.

We still feel that excluding a single request context from this set is artificial, mostly driven by what appears to be an implementation limitation, and detrimental to users from the point of view of least surprise and portability.

As such, users that want portability will still need to configure their application to specify the transaction context when moving from Quarkus to Open Liberty (if transaction propagation is even possible there, I don't recall), and to exclude transactions from being propagated when moving from OpenLiberty to Quarkus.

In other words, this proposed change doesn't seem to help users much, unless transactions are not involved at all, but in our experience, transactional requirements are not less common than security ones, so should get equal treatment when it comes to context propagation.

@manovotn
Copy link
Contributor

manovotn commented Jul 1, 2020

I agree that this (or a defined default set of contexts to propagate) is necessary for portability.

Sorry but I don't see this argument as valid. Introducing a minimum set doesn't make it portable. Imagine it were in place - then you have a user that "just uses defaults" in one implementation and that means he gets the minimum set + whatever else the implementation propagates (so in Quarkus that's transactional for instance). User app can easily (even indirectly) rely on other than minimum contexts to be propagated.
Then take this user and try to port him to other implementation (say Liberty). The default set is in place and works, but all other contexts can still be broken and user app won't work.

Minimum set only makes it portable under two conditions:

  • You only want to propagate what the set contains
  • You rely on all other contexts not being propagated otherwise you might be getting side effects in some implementations

A very simple way of making it portable is just using the MP Config to declare what exactly you want to be default. I don't see this as that problematic.

@njr-11
Copy link
Contributor

njr-11 commented Jul 1, 2020

Matej sums it up really well in pointing out,

Minimum set only makes it portable under two conditions:

* You only want to propagate what the set contains

* You rely on all other contexts _not_ being propagated otherwise you might be getting side effects in some implementations

A very simple way of making it portable is just using the MP Config to declare what exactly you want to be default. I don't see this as that problematic.

The disputed point is really over how typical it is for these two requirements to be met. With respect to the context types that are explicitly defined by the specification, it comes down to transaction context. Specifically, how often users are using ManagedExecutor or ThreadContext from within a global transaction (because if done outside of a global transaction, then propagation of the absence of a transaction reduces to clearing transaction and means the same thing). One group sees this being a common pattern and therefore finds little or no value in having a minimal set defined by the spec. Another group sees this as an uncommon pattern and is therefore pushing to improve usability for the majority of users by defining a minimal set in the spec that would spare these users the need to add the MP Config properties. Both of these seem like reasonable positions to me, and it really comes down to how the typical end user is actually using - and wanting to use (in the cases of MP Fault Tolerance, REST Client & Reactive) - MP Context Propagation and global transactions.

@Emily-Jiang
Copy link
Member Author

I won't speak for the FT folks, but in Rest Client, users can specify their own ExecutorService to use for asynchronous operations. So, the best practice for portability would be to specify an executor that propagates the contexts that they want. They should be able to achieve this easier when using the MP CP APIs, but could also achieve it using other APIs. But if they don't specify their own executor, the behavior will be different when they move from Open Liberty to Quarkus, or from Payara to TomEE, etc. - and if the behavior is going to be different in this case, I don't see much benefit from adding language to the Rest Client spec that implementations should delegate to MP CP. This would allow implementations to decide how (or if) they will propagate contexts. IMO, the only reason to add this language to the Rest Client spec would be if the MP CP spec specified a default set (or minimum default set) of contexts to propagate - then that would benefit Rest Client users with added out-of-the-box portability.

This sounds like you're almost asking for the contexts to be propagated even if MP-CP is not involved, to let implementations pick whatever way they want to achieve that goal.

IMO the goal of the integration (roughly "if MP-CP is available, then it should be used to propagate contexts to the underlying threads, if any are created") is to make it clear that 1/ context propagation will happen just like everywhere else where MP-CP is used, in a coherent manner, and 2/ it will be configurable via MP-CP.

That's already a lot of value.

As said by @andymc12, there are two caveats I can see.

  1. The integration is optional as Context Propagation is a standalone spec.

  2. Without a minimum set of contexts to be propagated, there is not much value to offer to end users.

As an end user, when I read the spec, I know this feature is optional. I need to find out which runtime to offer this. Another concern from the end user is that without a definite list context to be defined, he/she has no clue what will happen to his application. I don't see much value provided except more uncertainty. With the minimum set provided, at least, if the end user wants to propagate additional contexts, the mp.context.xxx property can be set to achieve portability.

@manovotn
Copy link
Contributor

manovotn commented Jul 7, 2020

Without a minimum set of contexts to be propagated, there is not much value to offer to end users.

I would argue the value (from PoV of portability) is exactly the same, e.g. there is no change. For a user that uses all currently defined contexts in MP, meaning transactional context is included (or deliberately excluded), you then have:

  • a scenario with minimum set - this is fine for all contexts but transactional; user that leverages all contexts the spec defines has to declare MP config property in order to achieve portability
  • a scenario without minimum set - in order to be able to make the application portable, user has to declare MP config property to set the default, otherwise a conflicting context (transactional ATM) will be propagated in one environament but not in another

Notably, introducing a minimum set then sets the requirement on any future implementation on the spec. The two we have now have already had issues agreeing on transactional; other impl may easily have issues with something like application context being propagated by default. So we might be setting unnecessary limitations for future impls here.

The value of using MP CP is still there though. When you say that propagation will be applied in accordance to what the MP CP impl declares, you now know you have a consistent propagation throughout the application wherever you use context propagation. User has two options:

  • Be lazy and just go with whatever the impl he currently uses does, potentially risking portability in the future. When that happens (if that even happens), it is still easily fixable by introducing the config.
  • Go ahead and declare the config right away, ensuring portability in the future

njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Jul 13, 2020
njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Jul 13, 2020
njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Jul 13, 2020
njr-11 added a commit to njr-11/microprofile-context-propagation that referenced this issue Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants