Skip to content

Commit 84e762b

Browse files
committed
Fix double accounting of JMS process observations
Prior to this commit, the instrumentation of the processing of JMS messages would happen a different levels of the hierarchy, accounting for alli known implementations, `SimpleMessageListenerContainer` and `DefaultMessageListenerContainer` as well as various use cases and `MessageListener` variants. Unfortunately, this instrumentation could lead to observing JMS processing twice in some cases, and would not be consistent about the scope of what's observed. This commit moves the instrumentation basics into the `AbstractMessageListenerContainer` but leaves the actual observation calls to the public implementations. Fixes gh-33758
1 parent e90a2da commit 84e762b

File tree

3 files changed

+17
-17
lines changed

3 files changed

+17
-17
lines changed

Diff for: spring-jms/src/main/java/org/springframework/jms/listener/AbstractMessageListenerContainer.java

+13-12
Original file line numberDiff line numberDiff line change
@@ -676,16 +676,21 @@ public void setupMessageListener(Object messageListener) {
676676
* @see #handleListenerException
677677
*/
678678
protected void executeListener(Session session, Message message) {
679-
createObservation(message).observe(() -> {
680-
try {
681-
doExecuteListener(session, message);
682-
}
683-
catch (Throwable ex) {
684-
handleListenerException(ex);
685-
}
686-
});
679+
try {
680+
doExecuteListener(session, message);
681+
}
682+
catch (Throwable ex) {
683+
handleListenerException(ex);
684+
}
687685
}
688686

687+
/**
688+
* Create, but do not start an {@link Observation} for JMS message processing.
689+
* <p>This will return a "no-op" observation if Micrometer Jakarta instrumentation
690+
* is not available or if no Observation Registry has been configured.
691+
* @param message the message to be observed
692+
* @since 6.1
693+
*/
689694
protected Observation createObservation(Message message) {
690695
if (micrometerJakartaPresent && this.observationRegistry != null) {
691696
return ObservationFactory.create(this.observationRegistry, message);
@@ -770,7 +775,6 @@ protected void doInvokeListener(SessionAwareMessageListener listener, Session se
770775

771776
Connection conToClose = null;
772777
Session sessionToClose = null;
773-
Observation observation = createObservation(message);
774778
try {
775779
Session sessionToUse = session;
776780
if (micrometerJakartaPresent && this.observationRegistry != null) {
@@ -782,7 +786,6 @@ protected void doInvokeListener(SessionAwareMessageListener listener, Session se
782786
sessionToClose = createSession(conToClose);
783787
sessionToUse = sessionToClose;
784788
}
785-
observation.start();
786789
// Actually invoke the message listener...
787790
listener.onMessage(message, sessionToUse);
788791
// Clean up specially exposed Session, if any.
@@ -794,11 +797,9 @@ protected void doInvokeListener(SessionAwareMessageListener listener, Session se
794797
}
795798
}
796799
catch (JMSException exc) {
797-
observation.error(exc);
798800
throw exc;
799801
}
800802
finally {
801-
observation.stop();
802803
JmsUtils.closeSession(sessionToClose);
803804
JmsUtils.closeConnection(conToClose);
804805
}

Diff for: spring-jms/src/main/java/org/springframework/jms/listener/SimpleMessageListenerContainer.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ protected void processMessage(Message message, Session session) {
341341
connectionFactory, new LocallyExposedJmsResourceHolder(session));
342342
}
343343
try {
344-
executeListener(session, message);
344+
createObservation(message).observe(() -> executeListener(session, message));
345345
}
346346
finally {
347347
if (exposeResource) {

Diff for: spring-jms/src/test/java/org/springframework/jms/listener/MessageListenerContainerObservationTests.java

+3-4
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,8 @@ void shouldRecordJmsProcessObservations(AbstractMessageListenerContainer listene
7575
JmsTemplate jmsTemplate = new JmsTemplate(connectionFactory);
7676
jmsTemplate.convertAndSend("spring.test.observation", "message content");
7777
latch.await(2, TimeUnit.SECONDS);
78-
assertThat(registry).hasObservationWithNameEqualTo("jms.message.process")
78+
assertThat(registry).hasNumberOfObservationsWithNameEqualTo("jms.message.process", 1)
79+
.hasObservationWithNameEqualTo("jms.message.process")
7980
.that()
8081
.hasHighCardinalityKeyValue("messaging.destination.name", "spring.test.observation");
8182
assertThat(registry).hasNumberOfObservationsEqualTo(1);
@@ -86,7 +87,6 @@ void shouldRecordJmsProcessObservations(AbstractMessageListenerContainer listene
8687
@ParameterizedTest(name = "[{index}] {0}")
8788
@MethodSource("listenerContainers")
8889
void shouldRecordJmsPublishObservations(AbstractMessageListenerContainer listenerContainer) throws Exception {
89-
CountDownLatch latch = new CountDownLatch(1);
9090
listenerContainer.setConnectionFactory(connectionFactory);
9191
listenerContainer.setObservationRegistry(registry);
9292
listenerContainer.setDestinationName("spring.test.observation");
@@ -100,8 +100,7 @@ void shouldRecordJmsPublishObservations(AbstractMessageListenerContainer listene
100100
TextMessage response = (TextMessage) jmsTemplate.sendAndReceive("spring.test.observation",
101101
session -> session.createTextMessage("test request"));
102102

103-
// request received by listener and response received by template
104-
assertThat(registry).hasNumberOfObservationsWithNameEqualTo("jms.message.process", 2);
103+
assertThat(registry).hasNumberOfObservationsWithNameEqualTo("jms.message.process", 1);
105104
// response sent to the template
106105
assertThat(registry).hasNumberOfObservationsWithNameEqualTo("jms.message.publish", 1);
107106

0 commit comments

Comments
 (0)