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

Use early static registration of EventPublishingContextWrapper #41439

Closed
wilkinsona opened this issue Jul 10, 2024 · 4 comments
Closed

Use early static registration of EventPublishingContextWrapper #41439

wilkinsona opened this issue Jul 10, 2024 · 4 comments
Assignees
Labels
type: task A general task
Milestone

Comments

@wilkinsona
Copy link
Member

@wilkinsona wilkinsona added this to the 3.4.x milestone Jul 10, 2024
@wilkinsona wilkinsona added the type: task A general task label Jul 10, 2024
@abounaime
Copy link

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
Copy link
Member Author

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

@philwebb philwebb self-assigned this Aug 12, 2024
@philwebb philwebb changed the title Review static registration of EventPublishingContextWrapper Use early static registration of EventPublishingContextWrapper Aug 21, 2024
@philwebb philwebb modified the milestones: 3.4.x, 3.4.0-M2 Aug 21, 2024
@mzeijen
Copy link
Contributor

mzeijen commented Aug 22, 2024

@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
Copy link
Member

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

wilkinsona added a commit that referenced this issue Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: task A general task
Projects
None yet
Development

No branches or pull requests

4 participants