Have a fully JMS 1.0 compatible instrumentation#11413
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d0d0f6e42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (Throwable ignored) { | ||
| ackMode = Session.AUTO_ACKNOWLEDGE; |
There was a problem hiding this comment.
Preserve JMS 1.0 session ack mode
When a JMS 1.0 provider uses QueueConnection.createQueueSession/TopicConnection.createTopicSession with transacted=true or Session.CLIENT_ACKNOWLEDGE, getAcknowledgeMode() is exactly the 1.1 method that raises AbstractMethodError, so this fallback records the SessionState as AUTO_ACKNOWLEDGE. JMSMessageConsumerInstrumentation only defers spans to acknowledge()/commit() when SessionState reports client-ack/transacted, and the Commit/Recover advices have the same guards, so these JMS 1.0 sessions now finish spans and time-in-queue batches with auto-ack semantics instead of the session's actual semantics. The fallback should preserve at least the transacted/client-ack mode from creation or getTransacted() rather than defaulting every error to AUTO.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This has been done on purpose - the tradeoff is to have not exact time here but still having 1.0 supported. Otherwise I would have needed to instrument also javax.jms.Connection (and all the subclasses) to capture that info when createSession/createTopicSession/createQueueSession are called. And that would have been made the instrumentation way heavier just to support few library versions. The tradeoff that we have in this PR is to me acceptable
a52fb9d to
04a3a80
Compare
What Does This Do
Few methods like
getDestinationorgetAcknoledgeModehave only been introduced with JMS 1.1. Even if muzzle guards against JMS 1.0 implementations, issues can arise when jms-api 1.1 is present in the classpath but some implementors offer a jms 1.0 implementations.This can happen for some old IBM MQ drivers. An example:
(Others errors also point to the JmsProducerInstrumentation).
This PR uses fallback methods to deal with JMS 1.0 implementors:
getDestinationThe tests have been conducted with a JmsConnectionFactory that wraps the original and simulate the absence of key methods used in the instrumentations (please not that I deliberately delegate some methods not existing in ConnectionFactory/TopicProducer/etc in order not to rewrite the full test since they are not used in the instrumentation itself)
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.