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

With new JMS Observability integration, JmsDefaultListenerContainerSpec could support a builder setter for new ObservationRegistry attribute #8734

Closed
nightswimmings opened this issue Sep 14, 2023 · 4 comments · Fixed by #8764

Comments

@nightswimmings
Copy link

Spring Boot is adding Observability and Traceability to JMS on the new micrometer-tracing layer.
So as soon as this is available in 3.2.0, we will miss a way to pass it in the JmsDefaultListenerContainerSpec builder when creating it like:

 Jms.container(jmsContainerFactory, topicDestination)
         .pubSubDomain(isTopic)
         .sessionAcknowledgeMode(Session.SESSION_TRANSACTED)
         .sessionTransacted(true)
          .cacheLevel(DefaultMessageListenerContainer.CACHE_AUTO)
          .taskExecutor(Executors.newCachedThreadPool())
          **.observarionRegistry(...)**
          .getObject()

See reference ticket: spring-projects/spring-framework#30335 (comment)

@nightswimmings nightswimmings added status: waiting-for-triage The issue need to be evaluated and its future decided type: enhancement labels Sep 14, 2023
@artembilan artembilan added in: jms and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Sep 14, 2023
@artembilan artembilan added this to the 6.2.0-M3 milestone Sep 14, 2023
@artembilan artembilan added the ideal-for-user-contribution An issue that would ideal for a user to get started with contributing. label Sep 14, 2023
@artembilan
Copy link
Member

So, the JmsListenerContainerSpec should be enhanced with an observarionRegistry() option to delegate to respective AbstractMessageListenerContainer.setObservationRegistry().

The contribution is welcome: https://github.com/spring-projects/spring-integration/blob/main/CONTRIBUTING.adoc!
Let us know if you are willing to do that!
Or @AdamaSorho will pick it up quickly 😉

@nightswimmings
Copy link
Author

I can try at home in the next days if you are not in much hurry!

@artembilan
Copy link
Member

Thanks.
Yes, technically, we are in a hurry for upcoming milestone next Tuesday, but since it is not a bug and there is indeed a workaround like I explained here: https://stackoverflow.com/questions/77107286/tracing-in-spring-integration-spring-boot-3-0-9, we are good to wait for the fix later on.

@artembilan
Copy link
Member

FYI, I have some related fix which might not require an interaction with listener container in Spring JMS: #8740.
Just because Spring Integration listener is, essentially, a consumer for of those JMS messages, therefore setting one or another observation is enough.
More over we have it pretty convenient on Spring Integration side: just @EnableIntegrationManagement(observationPatterns = "observedJms*") - and those listeners will be observed.

I don't say, thought, that we should not expose the mentioned option on a JmsListenerContainerSpec, which is, essentially, a builder for target AbstractMessageListenerContainer instance and really can be used outside of Spring Integration flows.

@artembilan artembilan modified the milestones: 6.2.0-M3, 6.2.0-RC1 Sep 19, 2023
@artembilan artembilan removed the ideal-for-user-contribution An issue that would ideal for a user to get started with contributing. label Oct 13, 2023
@artembilan artembilan self-assigned this Oct 13, 2023
artembilan added a commit to artembilan/spring-integration that referenced this issue Oct 13, 2023
Fixes spring-projects#8734

Expose `JmsListenerContainerSpec.observationRegistry(ObservationRegistry observationRegistry)` option
garyrussell pushed a commit that referenced this issue Oct 16, 2023
* GH-8734: expose JmsLisConSpec.observationRegistry

Fixes #8734

Expose `JmsListenerContainerSpec.observationRegistry(ObservationRegistry observationRegistry)` option

* * Fix Checkstyle violation for Javadoc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants