-
Notifications
You must be signed in to change notification settings - Fork 25
Add ContextSnapshot.captureFromMany variants #98
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
Conversation
Prior to this commit, `ContextSnapshot` would only allow to capture from a single context, unless the `captureAll` variants which involve `ThreadLocal` capture. This commit introduces new `captureFromMany` methods for this, since introducing varargs variants for `captureFrom` would break existing applications. This new method allows to capture a context snapshot from multiple contexts (without capturing from threadlocals). If a particular key is present in multiple contexts, it will be overriden as values are extracted.
This PR is in draft mode currently - there is a use case for this in Spring for GraphQL instrumentation. |
To clarify the scenario, the GraphQL Java |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall a straight forward change. In retrospect, the captureFrom
methods should have been vaarg methods from the start, just like captureAll
. Be it as it may, we can either double the number of captureFrom~
methods for a total of 6, or we can correct via deprecations. My thought is to rename the proposed captureFromMany
to captureFromContext
, and deprecate captureFrom
eventually to be removed.
What do you think @chemicL, @marcingrzejszczak ?
This commit introduces `captureFromContext` method variants and deprecates all `captureFrom` methods; this allows to introduce consistent ordering of method parameters and variants with varargs for supporting multiple contexts.
I've just pushed a second commit that aligns with Rossen's thoughts so you can see what it would look like. |
Looks good to me, thanks for aligning the method names, too. |
Prior to this commit, `ContextSnapshot` would only allow to capture from a single context, unless the `captureAll` variants which involve `ThreadLocal` capture. This commit introduces `captureFromContext` method variants with varargs to support multiple contexts and deprecates all `captureFrom` methods. See gh-98
Replaced by #105 |
Prior to this commit,
ContextSnapshot
would only allow to capture froma single context, unless the
captureAll
variants which involveThreadLocal
capture.This commit introduces new
captureFromMany
methods for this, sinceintroducing varargs variants for
captureFrom
would break existingapplications.
This new method allows to capture a context snapshot from multiple
contexts (without capturing from threadlocals). If a particular
key is present in multiple contexts, it will be overriden as values are
extracted.