Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Test utilities and scenarios for working with scopes #123

Merged
merged 12 commits into from
Jul 6, 2023

Conversation

chemicL
Copy link
Collaborator

@chemicL chemicL commented Jun 29, 2023

This change introduces a new module: context-propagation-test.

Update: This change revisits tests and utilities for working with values that are scoped. Leaving the original description for future reference how this change originated:

It will allow users of this library to validate their implementations against the expectations without needing to implement their own scoped value objects. Therefore, the module introduces:

  • ScopedValue - an interface and utility for working with values that maintain a scope hierarchy
  • ScopedValueThreadLocalAccessor - an accessor that can be used to register in the ContextRegistry to work with the above

Additionally, in order to make the module take advantage of the nullability related annotations from JSR 305, these annotations have been made public and were transferred to util.annotation package inside the context-propagation module.

The module is proposed as a consequence of reactor/reactor-core#3516 which can be simplified to only implement the domain-specific tests and use the new module instead of implementing the ScopedValue, which is a generic one. It also decouples the project from a copy of Micrometer's Observation-related classes.

To further clarify the motivations:

Let's assume the following layers:

  • <0> something that propagates ThreadLocals (context-propagation library) - ContextSnapshot
  • <1> something that manages ContextSnapshots and ContextSnapshot.Scopes (e.g. reactor-core)
  • <2> something that creates a ThreadLocalAccessor (e.g. micrometer-observation)

The introduced test module will help layer <0> as the tests for it belong to layer <1>; in case of layer <1> libraries, this proposal gives means to test scoped scenarios that are decoupled from any particular use case. Layer <2> will work with their respective implementation anyway.

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 great to see this emerging for better testing of Micrometer scenarios without actually bringing in all of Micrometer. That said it looks more for internal consumption in context-propagation and Reactor tests than for general use. I think it makes more sense to keep it private and defer creating a test module.

Also some feedback on a more detailed level.

There is a circular relationship between ScopeValue and Scope that makes it harder to understand the API. On the one hand, ScopeValue contains the declaration of Scope, and has Scope instance methods. On the other hand, Scope contains ScopeValue.

It's worth considering if it is possible to break the cycle. I haven't tried in earnest, but I think it may have to do with having methods that operate on ThreadLocals mixed in with the main abstractions, which is a convenience but creates a tangle and obscures the structural relationship of the domain types.

SimpleScopedValue is mainly a container for a String value with a get() to obtain it. The remaining methods that operate on ThreaLocal<Scope> could be probably separated out into a separate class, something like ScopeHolder and that in turn could mean that ScopeValue as a contract doesn't need to expose Scope instance methods which are in any case almost entirely static in behavior.

While not causing a tangle, maybe ThreadLocal<ScopedValue> also doesn't need to be on ScopedValue itself, and move to a ScopeValueHolder or maybe one holder for Scope and ScopeValue thread locals.

@chemicL
Copy link
Collaborator Author

chemicL commented Jul 4, 2023

Thank you for the brilliant feedback @rstoyanchev :) To my surprise, in the end the classes look almost exactly the same as the simplified Observation classes. I could get rid of the Observation classes then and adapt the older tests to the new names.

@chemicL chemicL changed the title context-propagation-test module Test utilities and scenarios for working with scopes Jul 4, 2023
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.

Looks much simpler 👍

I experimented with moving ScopeValue#open as a static method on Scope, and also moved Scope out of ScopeValue. The usage doesn't seem more inconvenient. In fact no longer have to have ScopeValue.Scope everywhere, and as a result, it makes ScopeValue nothing but a simple value container breaking the two way dependency. See rstoyanchev@a1f42f5.

In addition, the scopedvalue is defined in a sub-package but it is in fact a higher level package. Perhaps it would be better as io.micrometer.scopedvalue. After all it is meant to emulate something that's outside of context-propagation and itself is not aware of context-propagation.

@chemicL chemicL merged commit 75c5bca into 1.0.x Jul 6, 2023
@chemicL chemicL deleted the context-propagation-test branch July 6, 2023 12:42
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.0.x, 1.0.4 Jul 7, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants