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

Introducing ContextSnapshotFactory #105

Merged
merged 16 commits into from Jun 5, 2023
Merged

Introducing ContextSnapshotFactory #105

merged 16 commits into from Jun 5, 2023

Conversation

chemicL
Copy link
Collaborator

@chemicL chemicL commented May 26, 2023

This change is mainly allowing clearing ThreadLocals upon scope enter.

There are however more changes to the API:

  • Added a ContextSnapshotFactory concept, that allows configuring with a ContextRegistry, the Predicate to use when capturing, and a boolean flag whether missing mappings in a context object should clear existing ThreadLocals or not touch them.
  • Deprecated existing static methods in ContextSnapshot and made them use a global instance of ContextSnapshotFactory which does not clear missing values.
  • Forked the DefaultContextSnapshot test to cover the deprecated methods.
  • Used a ContextSnapshotFactory.Builder in tests to depict the intended usage.

@chemicL chemicL added the enhancement A general enhancement label May 26, 2023
@chemicL chemicL self-assigned this May 26, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented May 26, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/context-propagation/105.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/micrometer-metrics/context-propagation/105.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

Copy link
Collaborator

@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.

Maybe we can take the opportunity to streamline the API a little.

For a start, I think we can collect a keyPredicate in the builder, in effect expecting the factory to be pre-configured with it rather than having them passed in. Capturing tends to be a lot more fixed and inclusive, so I expect this should be just fine.

Then for the methods:

ContextSnapshot capture(Object... contexts);
ContextSnapshot captureFromContext(Object... contexts); 
<C> ContextSnapshot.Scope setThreadLocalsFrom(Object sourceContext, String... keys);

For setThreadLocalsFrom if the keys is empty, set all.

@chemicL
Copy link
Collaborator Author

chemicL commented May 31, 2023

@rstoyanchev thanks for the suggestion. I applied the change to the draft, let me know your thoughts. Keep in mind the DefaultContextSnapshot and DefaultContextSnapshotFactory implementations need to provide backwards compatibility to allow for the deprecated methods to behave as before.

@rstoyanchev
Copy link
Collaborator

One more thing, I see the setThreadLocalsFrom static methods are moved to DefaultContextSnapshotFactory but not the capture static methods. I think it would make sense to have that logic in the factory.

Copy link
Collaborator

@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.

Thanks for applying the changes.

I think there may be some further opportunity to consolidate logic across the setThreadLocals methods, but to minimize moving parts, we can keep it at that for now, and try another pass after the PR is in.

Some minor code ordering comments below.

@chemicL chemicL changed the title [WIP] ContextSnapshotFactory allowing clearing ThreadLocals upon scope enter Introducing ContextSnapshotFactory Jun 2, 2023
@chemicL chemicL marked this pull request as ready for review June 2, 2023 13:54
@chemicL chemicL requested a review from rstoyanchev June 2, 2023 14:53
Copy link
Collaborator

@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.

Overall looks good. Some minor suggestions below.

chemicL and others added 2 commits June 5, 2023 11:08
…tSnapshotFactory.java

Co-authored-by: Rossen Stoyanchev <rstoyanchev@users.noreply.github.com>
@chemicL chemicL requested a review from rstoyanchev June 5, 2023 09:24
@chemicL chemicL merged commit 9b3ecd4 into 1.0.x Jun 5, 2023
7 checks passed
@chemicL chemicL deleted the context-snapshot-factory branch June 5, 2023 10:20
@chemicL chemicL added this to the 1.0.3 milestone Jun 7, 2023
@chemicL chemicL added the release notes Noteworthy change to call out in the release notes label Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement release notes Noteworthy change to call out in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants