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

Add ContextPropagation runtime util + captureContext operator #3145

Merged
merged 11 commits into from
Oct 6, 2022

Conversation

simonbasle
Copy link
Member

@simonbasle simonbasle commented Aug 10, 2022

In this PR we introduce a ContextPropagation runtime-detection utility class
as well as a contextCapture() operator. If context-propagation isn't on the
classpath, this operator is NO-OP.

If context-propagation is on the classpath however, the operator will use
it to capture the current context during the subscription phase (at the point
a contextWrite would be effected) and store it in the ContextView visible
from upstream of the operator.

TODO:

  • Mono version
  • mention in reference guide? whole section?
  • marble diagram

@simonbasle simonbasle marked this pull request as ready for review August 16, 2022 08:56
@simonbasle
Copy link
Member Author

as there is currently no standard marble diagram representation of the Context, we won't provide a marble diagram for now

Copy link
Contributor

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some minor comments on the docs.

docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
@simonbasle
Copy link
Member Author

tentative commit message:

This commit introduces a ContextPropagation runtime-detection utility
class as well as a `contextCapture()` operator.

If context-propagation isn't on the classpath, this operator is NO-OP.

If context-propagation is on the classpath however, the operator will
use it to capture the current context during the subscription phase
(at the point a contextWrite would be effected) and store it in the
ContextView visible from upstream of the operator.

@simonbasle simonbasle requested a review from a team September 1, 2022 15:06
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
docs/asciidoc/advanced-contextPropagation.adoc Outdated Show resolved Hide resolved
* inject them into the {@link Context}
*/
public final Mono<T> contextCapture() {
if (!ContextPropagation.isContextPropagationAvailable()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remarks as in case of Flux.

private static final AtomicReference<String> REF1 = new AtomicReference<>();
private static final AtomicReference<String> REF2 = new AtomicReference<>();

//NOTE: no way to currently remove accessors from the ContextRegistry, so we recreate one on each test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chemicL
Copy link
Member

chemicL commented Sep 9, 2022

Approved, although I left a few comments - some are minor, for the docs; one I believe to be important for UX, but possible to improve as a follow-up if the logging option is chosen. With additional method, it's probably best to introduce now.

@OlegDokuka OlegDokuka modified the milestones: 3.5.0-M6, 3.5.0-M7 Sep 14, 2022
@simonbasle
Copy link
Member Author

simonbasle commented Oct 5, 2022

looks like there was no strong consensus on the contextCapture operator being NO-OP if the lib is not on the classpath.
so far all the features in reactor-core that depend on an optional dependency have been exposed that way (eg. Flux#metrics()).

I've fixed the comments on the refguide part.

Note that this PR is conflicting with #3215, as it adds usage of context-propagation including methods that have seen breaking changes in latest snapshots... this is just out-of-sync with M5

simonbasle and others added 4 commits October 5, 2022 16:12
Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
@simonbasle
Copy link
Member Author

I'll need to rebase against current main (which depends on context-propagation M5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants