search

spring-projects / spring-boot

Spring Boot
https://spring.io/projects/spring-boot
Apache License 2.0
74.81k stars 40.6k forks source link

Use early static registration of EventPublishingContextWrapper #41439

Closed wilkinsona closed 1 month ago

wilkinsona commented 3 months ago

https://github.com/spring-projects/spring-boot/blob/a34917007308fbcd22028c2a39522a4fad5e9651/spring-boot-project/spring-boot-actuator-autoconfigure/src/main/java/org/springframework/boot/actuate/autoconfigure/tracing/OpenTelemetryAutoConfiguration.java#L168

abounaime commented 3 months ago

Hi @wilkinsona,

I would like to work on this issue. Could you please assign it to me?

Additionally, I have a few questions to ensure I address the issue correctly:

  1. Is the main goal of this task to ensure that the static registration of EventPublishingContextWrapper is thread-safe and idempotent?
  2. Are there any specific aspects or additional requirements that I should focus on while reviewing the registration process?

Thanks for your guidance! Best regards,

wilkinsona commented 3 months ago

Thanks for the offer, @abounaime, but we'd prefer to handle this one ourselves.

mzeijen commented 1 month ago

@philwebb I looked at this fix and I think it will work great. There is only one situation where it won't work, and that is for a specific case when running unit tests. I encountered this in my own, similar, workaround for this early context wrapper initialization problem.

The situation is as follows: When a unit test runs first, that doesn't use Spring Boot Test, which accesses the OpenTelemetry context, then it will cause the Context wrappers to have been applied before the OpenTelemetryEventPublisherApplicationListener was run. When a following test does use Spring Boot Test, running the OpenTelemetryEventPublisherApplicationListener, the ContextStorage.addWrapper won't have any effect.

I solved this by implementing a JUnit org.junit.platform.launcher.TestExecutionListener that triggers that the delegate wrapper is added to the context wrappers. It might be good to consider implementing something similar, or at least provide a public method with which users, as myself, can trigger the adding of the context wrapper.

If you want me to create a new issue for this, then I will happily do so.

philwebb commented 1 month ago

Thanks for trying it @mzeijen, I've opened #42005 based on your feedback.