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

NEWRELIC-3834 add embedded tomcat jmx instrumentation #1039

Merged
merged 1 commit into from
Nov 12, 2022

Conversation

tbradellis
Copy link
Contributor

Add instrumentation to cover embedded tomcat.
The agent doesn’t correctly query the following Tomcat JMX metrics when using embedded Tomcat with SpringBoot:

type=DataSource
type=ThreadPool
type=Manager

We add Tomcat JMX values for the above categories if the server was started via one of the following two methods:

org.apache.catalina.startup.HostConfig.start()
org.apache.catalina.core.StandardServer.startInternal()

It appears that when using embedded Tomcat with Spring Boot StandardServer does get instrumented properly:

Supportability/WeaveInstrumentation/WeaveClass/com.newrelic.instrumentation.tomcat-jmx/org/apache/catalina/core/StandardServer
Supportability/WeaveInstrumentation/WeaveClass/com.newrelic.instrumentation.tomcat-8.5.2/org/apache/catalina/core/StandardServer

By default, embedded Tomcat JMX is disabled by Spring Boot. In application.properties Tomcat mbeans need to be enabled by adding: server.tomcat.mbeanregistry.enabled=true

The problem though is that the Mbean group namespaces are different, standalone Tomcat uses the prefix Catalina for queries whereas embedded Tomcat uses the prefix Tomcat. Our tomcat-jmx instrumentation only queries MBeans with the Catalina prefix thus queries initiated by embedded Tomcat fail due to an incorrect group name.

Standalone Tomcat JMX:

Embedded Tomcat JMX

For now, the recommended workaround would be to utilize Custom JMX instrumentation by YAML and create a dashboard to view the metrics.

Overview

Describe the changes present in the pull request

Related Github Issue

Include a link to the related GitHub issue, if applicable

Testing

run local tests against embedded tomcat to confirm metrics are collected

Checks

@tbradellis tbradellis changed the title add embedded tomcat jmx instrumentation NEWRELIC-3834 add embedded tomcat jmx instrumentation Oct 15, 2022
@tbradellis tbradellis marked this pull request as draft October 15, 2022 21:53
@tbradellis tbradellis marked this pull request as ready for review October 15, 2022 21:53
@tbradellis
Copy link
Contributor Author

tbradellis commented Oct 16, 2022

Do not merge. There's still an issue with the DataSource metrics. They are not reported as expected, though Manager and ThreadPool are the same as the standard tomcat instrumentation.
Resolved

@tbradellis tbradellis marked this pull request as draft October 16, 2022 23:00
@tbradellis tbradellis force-pushed the embedded-tomcat-jmx branch 2 times, most recently from b6d9fe4 to 48ec53c Compare October 18, 2022 04:36
@tbradellis tbradellis marked this pull request as ready for review October 18, 2022 04:36
@tbradellis
Copy link
Contributor Author

tbradellis commented Oct 18, 2022

Tested locally with Spring Boot and embedded Tomcat.
Requires extra config in Spring Boot:

spring.datasource.tomcat.jmx-enabled=true
server.tomcat.mbeanregistry.enabled=true
spring.datasource.type=org.apache.tomcat.jdbc.pool.DataSource

Visually confirmed that the correct charts on JVMs page light up.

//found
try {
if (server.queryNames(new ObjectName("Tomcat:type=Server"), null).size() == 1) {
AgentBridge.jmxApi.addJmxMBeanGroup(JMX_EMBEDDED_PREFIX);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Embedded tomcat will have
Tomcat:type=Server
Non-embedded will have:
Catalina:type=Server

Copy link
Contributor

@jtduffy jtduffy left a comment

Choose a reason for hiding this comment

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

+1

update copyright

different name for datasource mbean

reorganize tomcat jmx

fix tomcat utils

check for embedded signal

clarify comment for non embedded tc

log different paths by name

also check datasource b4 adding jmx

clarify comment
@tbradellis tbradellis merged commit 19245c6 into main Nov 12, 2022
@tbradellis tbradellis deleted the embedded-tomcat-jmx branch November 12, 2022 01:27
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.

2 participants