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
Introduce ObservationThreadLocalTestExecutionListener
in the TestContext framework
#30651
Introduce ObservationThreadLocalTestExecutionListener
in the TestContext framework
#30651
Conversation
without this change we have no option to pass the ObservationRegistry that is registered in the given TestContext's ApplicationContext as the one that should be used by the ObservationThreadLocalAccessor. with this change we create a TestExecutionListener that will set an ObservationRegistry registered in the TestContext's ApplicationContext for each test class and after each test class will restore the previously stored ObservationRegistry instance
cc @rstoyanchev after upgrading the context propagation library I needed to update one class |
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.
I'm only commenting on the change in spring-webmvc. I have not reviewed the spring-test changes.
...src/main/java/org/springframework/web/servlet/mvc/method/annotation/ReactiveTypeHandler.java
Outdated
Show resolved
Hide resolved
ObservationThreadLocalTestExecutionListener
in the TestContext framework
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.
Thanks for the PR.
I've made an initial review and requested several changes.
I've also raised a few points that need to be decided on.
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
* This implementation is not thread-safe. | ||
* | ||
* @author Marcin Grzejszczak | ||
* @since 6.0 |
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.
* @since 6.0 | |
* @since 6.1 |
I imagine this would be added in 6.1.
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.
We've made the whole train of changes in Micrometer Core (1.10.8), Context Propagation (1.0.3), Micrometer Tracing (1.0.7) and Reactor all in patch versions to fix bugs related to context propagation with reactor. We need this to be working in a patch release IMO. Without this change context propagation will not work properly. I would say that adding this component is a bug fix not an enhancement.
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
spring-test/src/main/java/org/springframework/test/context/observation/package-info.java
Outdated
Show resolved
Hide resolved
...va/org/springframework/test/context/observation/ObservationThreadLocalTestListenerTests.java
Outdated
Show resolved
Hide resolved
...in/java/org/springframework/test/context/observation/ObservationThreadLocalTestListener.java
Outdated
Show resolved
Hide resolved
…ervation/ObservationThreadLocalTestListener.java Co-authored-by: Sam Brannen <sam@sambrannen.com>
…ervation/ObservationThreadLocalTestListener.java Co-authored-by: Sam Brannen <sam@sambrannen.com>
…ervation/ObservationThreadLocalTestListener.java Co-authored-by: Sam Brannen <sam@sambrannen.com>
…ervation/package-info.java Co-authored-by: Sam Brannen <sam@sambrannen.com>
…ervation/ObservationThreadLocalTestListenerTests.java Co-authored-by: Sam Brannen <sam@sambrannen.com>
- dependencies ordered alphabetically - renamed the test execution listener to contain "execution" and "micrometer" in the name - production code lines are smaller (up to 100 chars) - test execution attribute is a private static final field - added javadocs in the test execution listener
Without this change we have no way to pass the
ObservationRegistry
that is registered in the given test'sApplicationContext
as the one that should be used by theObservationThreadLocalAccessor
.With this change we introduce a
TestExecutionListener
that will set anObservationRegistry
registered in the test'sApplicationContext
for each test class and after each test class will restore the previously storedObservationRegistry
instance.