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

Add (more) Spring JMS support #6308

Merged
merged 4 commits into from
Jul 13, 2022
Merged

Add (more) Spring JMS support #6308

merged 4 commits into from
Jul 13, 2022

Conversation

trask
Copy link
Member

@trask trask commented Jul 12, 2022

Adds support for org.springframework.jms.listener.SessionAwareMessageListener direct listener usage.

I tried to repro with @JmsListener that accepts Session as second arg, but Spring tunnels that through normal 2-arg JMS MessageListener (the one with Session arg), which is already instrumented by JMS instrumentation.

Open question

I copy-pasted a bunch of code from the jms-1.1 instrumentation to the new spring-jms-2.0 instrumentation.

I could alternatively add a direct dependency from spring-jms-2.0 instrumentation on jms-1.1 instrumentation.

I'm a little worried about the DbInfo class loader problem, of having the same classes referenced from two different class loaders.

But I guess not, since that was only a problem from passing DbInfo in between those two class loaders, which I don't think would happen here since nothing would be passed between the two instrumentation, just pure reuse of classes from jms-1.1.

@laurit wdyt?

context.close()

where:
config << [AnnotatedListenerConfig, ManualListenerConfig]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the existing test (which passed with just JMS listener instrumentation) was AnnotatedListenerConfig, and the new test that exercises the new instrumentation is ManualListenerConfig

@laurit
Copy link
Contributor

laurit commented Jul 12, 2022

@trask I think that adding a dependency on jms-1.1 is fine.

@trask trask marked this pull request as ready for review July 12, 2022 23:42
@trask trask requested a review from a team July 12, 2022 23:42
@Override
public void transform(TypeTransformer transformer) {
transformer.applyAdviceToMethod(
named("onMessage").and(takesArguments(2)).and(isPublic()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Message is used in the advice

Suggested change
named("onMessage").and(takesArguments(2)).and(isPublic()),
named("onMessage").and(isPublic()).and(takesArguments(2)).and(takesArgument(0, named("javax.jms.Message"))),

@trask trask merged commit 2a59d0f into open-telemetry:main Jul 13, 2022
@trask trask deleted the spring-jms branch July 13, 2022 22:37
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 23, 2022
* Add (more) Spring JMS support

* Remove duplication

* Better advice matcher
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Oct 31, 2022
* Add (more) Spring JMS support

* Remove duplication

* Better advice matcher
LironKS pushed a commit to helios/opentelemetry-java-instrumentation that referenced this pull request Dec 4, 2022
* Add (more) Spring JMS support

* Remove duplication

* Better advice matcher
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

Successfully merging this pull request may close these issues.

3 participants