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

Allow opt-out for MessageListener matcher performance optimization #3135

Closed
JonasKunz opened this issue May 4, 2023 · 2 comments · Fixed by #3299
Closed

Allow opt-out for MessageListener matcher performance optimization #3135

JonasKunz opened this issue May 4, 2023 · 2 comments · Fixed by #3299
Labels
agent-java enhancement Enhancement of an existing feature
Milestone

Comments

@JonasKunz
Copy link
Contributor

#2930 added a performance optimization by pre-filtering instrumentation candidates for our MessageListener instrumentation.
Unfortunately, there is currently no way of restoring the old behaviour: It is not possible to make the agent scan all packages for MessageListeners. If a user has no knowledge of where the MessageListeners are placed, he has no chance of instrumenting them.

A simple, backwards-compatible fix would be to enhance the elastic.apm.jms_listener_packages config option to accept wildcard matchers instead of strings. This way, a user can set jms_listener_packages=* to make the agent scan all packages.

A downside of this approach is a bit of inconsistency in the configuration: Other wildcard options such as classes_excluded_from_instrumentation match the wildcards against the entire class name, whereas jms_listener_packages only checks that the classname begins with the provided pattern / string.

@JonasKunz JonasKunz added enhancement Enhancement of an existing feature 8.10-candidate labels May 4, 2023
@AlexanderWert AlexanderWert added this to the 8.10 milestone Jun 26, 2023
@JonasKunz
Copy link
Contributor Author

We had another user stumble over problems due to this config option, see this comment.

We should therefore also do the following:

  • Clarify the documentation about the behavior when jms_listener_packages is not set.
  • Ensure that the application_packages configuration value is also always scanned for MessageListeners: This would already have avoided the problem in your case

@AlexanderWert AlexanderWert modified the milestones: 8.10, 8.11 Aug 7, 2023
@SylvainJuge
Copy link
Member

Regarding this option, we have the following things that we could change:

  1. update documentation to better explain the default heuristic
  2. include the application_packages in addition to the values set in jms_listener_packages
  3. add wildcard support to jms_listener_packages that would allow users to set jms_listener_packages=* to help discover what needs to be configured without prior knowledge of the application internals.
  4. change (or add another) config option to jms_listener_classes that would be a comma-separated list of wildcards, the default value would match the current heuristic with classes that have $$, Message or Listener.
  5. broaden the default heuristic to contain Bean or case variants of EJB which would also have helped with this issue.

I think that the points 1, 2 and 5 are quite straightforward to implement and do not introduce any inconsistency or significant change in the documentation.

For 3 (adding wildcard support), I think that it would just allow to provide a way to discover for the JMS listeners, whereas the same could apply to all the places where the application_packages is used for pre-filtering (we do that for our annotations).
I think that for the use-case of "application internals discovery" just disabling the pre-filtering would work as well and could be a documented strategy for people that need to configure the agent by inspecting the logs. For now this has mostly been used for temporary work-around or debugging.
Furthermore, when combined with 2, we could just document doing this for application_packages as the configured value would be inherited in jms_listener_packages, we could then deprecate this option and instruct people to only use `application_packages".

For 4 (adding another option), it will likely complicate things further thus it's not something that I would suggest doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-java enhancement Enhancement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants