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

spring-jms support #778

Open
codefromthecrypt opened this issue Sep 1, 2018 · 8 comments
Open

spring-jms support #778

codefromthecrypt opened this issue Sep 1, 2018 · 8 comments

Comments

@codefromthecrypt
Copy link
Member

#584 added JMS support, including message listeners. Not all libraries use JMSConsumer.setMessageListener(listener) or MessageConsumer.setMessageListener(listener). Notably, spring supports other means to invoke. This means we'll need to provide hooks to wrap similarly when in Spring.

This likely means handling the following:

  • JmsMessageEndpointManager - implicitly backfilling JCA connector tests
  • DefaultMessageListenerContainer
  • SimpleMessageListenerContainer
@codefromthecrypt
Copy link
Member Author

codefromthecrypt commented Sep 1, 2018

when we do this we have to be very careful when wrapping as there is dispatch logic like this:

package org.springframework.jms.config;

public abstract class AbstractJmsListenerEndpoint implements JmsListenerEndpoint {
	@Override
	public void setupListenerContainer(MessageListenerContainer listenerContainer) {
		if (listenerContainer instanceof AbstractMessageListenerContainer) {
			setupJmsListenerContainer((AbstractMessageListenerContainer) listenerContainer);
		}
		else {
			new JcaEndpointConfigurer().configureEndpoint(listenerContainer);
		}
	}

For example, the type AbstractMessageListenerContainer is inspected, so we shouldn't wrap using that type unless the delegate is also that type!

@codefromthecrypt
Copy link
Member Author

PS the TracingMessageListener which we use internally in JmsTracing won't be used for something like Spring in the same way. the TracingMessageListener we have in normal JMS starts both a producer and a consumer span. In the case of Spring, it is calling "receive" hence has already made a consumer span: we only want to create a child and scope that span around the invoked listener.

Our tests should prove there is only one CONSUMER span in other words, to ensure no double-instrumentation.

https://github.com/openzipkin/sleuth-webmvc-example/compare/add-jms-tracing

screen shot 2018-08-31 at 4 54 09 pm

@codefromthecrypt
Copy link
Member Author

to the point about dual hierarchy:

java.lang.IllegalArgumentException: Could not configure endpoint with the specified container 'sleuth.webmvc.JmsConfiguration$TracingForwardingJmsListenerEndpoint$1@6ef2f7ad' Only JMS (org.springframework.jms.listener.AbstractMessageListenerContainer subclass) or JCA (org.springframework.jms.listener.endpoint.JmsMessageEndpointManager) are supported.

@codefromthecrypt
Copy link
Member Author

added a commit to https://github.com/openzipkin/sleuth-webmvc-example/compare/add-jms-tracing which adds receiver spans for basic message listeners. Will test more later

@codefromthecrypt
Copy link
Member Author

branch now works with all listeners except JCA. will try to wrap that up tomorrow.. We also need to test topic subscription to ensure there are no state problems (like listeners getting exact same instance of JMS Message and therefore getting into overlap writes trouble)

@varpa89
Copy link

varpa89 commented Jan 25, 2019

@adriancole any news?

@frankpolkm
Copy link

Checkout the add-jms-tracing branch, the jms sending is OK, but listener doesn't work. Also, need add spring.main.allow-bean-definition-overriding=true to start the backend. Would you please provide a working sample for the 2.1.0 release?

@codefromthecrypt
Copy link
Member Author

@frankpolkm can you clarify if you are primarily interested in usage via sleuth or brave directly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants