Skip to content

Jmx unit semconv alignment - Tomcat #13650

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion instrumentation/jmx-metrics/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ No targets are enabled by default. The supported target environments are listed
- [camel](javaagent/camel.md)
- [jetty](javaagent/jetty.md)
- [kafka-broker](javaagent/kafka-broker.md)
- [tomcat](javaagent/tomcat.md)
- [tomcat](library/tomcat.md)
- [wildfly](javaagent/wildfly.md)
- [hadoop](javaagent/hadoop.md)

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@
import java.util.Set;
import org.junit.jupiter.api.Test;

/**
* TODO: This test will eventually go away when all yaml files are moved from javaagent to library
* directory. When yaml file is moved from javaagent to library then appropriate item must be
* removed from JmxMetricInsightInstallerTest#FILES_TO_BE_TESTED and corresponding test must be
* added in the library.
*/
class JmxMetricInsightInstallerTest {
private static final String PATH_TO_ALL_EXISTING_RULES = "src/main/resources/jmx/rules";
private static final Set<String> FILES_TO_BE_TESTED =
Expand All @@ -32,7 +38,6 @@ class JmxMetricInsightInstallerTest {
"hadoop.yaml",
"jetty.yaml",
"kafka-broker.yaml",
"tomcat.yaml",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[for reviewer] Tomcat metrics definition is tested in a much better way in TomcatIntegrationTest.java

Copy link
Contributor

Choose a reason for hiding this comment

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

By "much better way" here it means being tested against a real tomcat instance rather than trying to parse the yaml file to see if there are any errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moreover, it can now be easily tested with multiple tomcat versions

"wildfly.yaml"));

@Test
Expand Down
13 changes: 0 additions & 13 deletions instrumentation/jmx-metrics/javaagent/tomcat.md

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
---
# For Tomcat, the default JMX domain is "Catalina:", however with some deployments like embedded in spring-boot
# we can have the "Tomcat:" domain used, thus we use both MBean names for the metrics.

rules:
- beans:
- Catalina:type=GlobalRequestProcessor,name=*
- Tomcat:type=GlobalRequestProcessor,name=*
prefix: tomcat.
metricAttribute:
tomcat.request_processor.name: param(name)
mapping:
errorCount:
metric: error.count
type: counter
unit: "{error}"
desc: The number of errors.
requestCount:
metric: request.count
type: counter
unit: "{request}"
desc: The number of requests processed.
maxTime:
metric: request.duration.max
type: gauge
sourceUnit: ms
unit: s
desc: The longest request processing time.
processingTime:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that processingTime and maxTime mbean attributes are related, so we should probably make sure their name reflect that, for example:

  • tomcat.request.max.time for maxTime + tomcat.request.total.time for processingTime
  • having the total or max not at the end of the metric name helps to avoid the "do not use total" rule in semconv
  • alternatively, we could argue that the "do not use total" rule in semconv refers only to the _total and not total suffix and then use tomcat.request.time.max and tomcat.request.time.total but this might be harder to make people agree on this.

metric: request.processing_time
type: counter
sourceUnit: ms
unit: s
desc: Total time for processing all requests.
bytesReceived:
metric: &metric network.io
type: &type counter
unit: &unit By
desc: &desc The number of bytes transmitted.
metricAttribute:
network.io.direction: const(receive)
bytesSent:
metric: *metric
type: *type
unit: *unit
desc: *desc
metricAttribute:
network.io.direction: const(transmit)

- beans:
- Catalina:type=Manager,host=localhost,context=*
- Tomcat:type=Manager,host=localhost,context=*
prefix: tomcat.
metricAttribute:
tomcat.context: param(context)
mapping:
activeSessions:
metric: active_session.count
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metric: active_session.count
metric: session.active.count

In addition to renaming I would suggest to add tomcat.session.active.limit updowncounter as it provides the configured limit if there is any or -1 if none is set, so using the negative value filtering would be appropriate for it.

As a side note, there are also the expiredSessions and rejectedSessions mbeans attributes that would be tempting to use for a tomcat.session.count with a metric attribute to provide breakdown per state on the total number of sessions but we would be making assumptions on the implementation, so I would not recommend that, also it prevents us from having tomcat.session.active.count and tomcat.session.active.limit.

type: updowncounter
unit: "{session}"
desc: The number of active sessions.

- beans:
- Catalina:type=ThreadPool,name=*
- Tomcat:type=ThreadPool,name=*
unit: "{thread}"
prefix: tomcat.thread.
type: updowncounter
metricAttribute:
tomcat.thread_pool.name: param(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tomcat.thread_pool.name: param(name)
tomcat.thread.pool.name: param(name)

This would be consistent with db.client.connection.pool.name defined in DB semconv.

mapping:
currentThreadCount:
metric: count
desc: Total thread count of the thread pool.
currentThreadsBusy:
metric: busy.count
desc: Number of busy threads in the thread pool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could also capture maxThreads as tomcat.thread.limit (updowncounter), but this could be done in another PR as an improvement.

Original file line number Diff line number Diff line change
@@ -0,0 +1,138 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.jmx.rules;

import static io.opentelemetry.instrumentation.jmx.rules.assertions.DataPointAttributes.attribute;
import static io.opentelemetry.instrumentation.jmx.rules.assertions.DataPointAttributes.attributeGroup;
import static io.opentelemetry.instrumentation.jmx.rules.assertions.DataPointAttributes.attributeWithAnyValue;

import io.opentelemetry.instrumentation.jmx.rules.assertions.AttributeMatcher;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.containers.wait.strategy.Wait;

public class TomcatIntegrationTest extends TargetSystemTest {

@ParameterizedTest
@CsvSource({
"tomcat:10.0, https://tomcat.apache.org/tomcat-10.0-doc/appdev/sample/sample.war",
"tomcat:9.0, https://tomcat.apache.org/tomcat-9.0-doc/appdev/sample/sample.war"
})
void testCollectedMetrics(String dockerImageName, String sampleWebApplicationUrl)
throws Exception {
List<String> yamlFiles = Collections.singletonList("tomcat.yaml");

yamlFiles.forEach(this::validateYamlSyntax);

List<String> jvmArgs = new ArrayList<>();
jvmArgs.add(javaAgentJvmArgument());
jvmArgs.addAll(javaPropertiesToJvmArgs(otelConfigProperties(yamlFiles)));

// testing with a basic tomcat image as test application to capture JVM metrics
GenericContainer<?> target =
new GenericContainer<>(dockerImageName)
.withEnv("CATALINA_OPTS", String.join(" ", jvmArgs))
.withStartupTimeout(Duration.ofMinutes(2))
.withExposedPorts(8080)
.waitingFor(Wait.forListeningPorts(8080));

copyFilesToTarget(target, yamlFiles);

startTarget(target);

// Deploy example web application to the tomcat to enable reporting tomcat.active_session.count
// metric
target.execInContainer("rm", "-fr", "/usr/local/tomcat/webapps/ROOT");
target.execInContainer(
"curl", sampleWebApplicationUrl, "-o", "/usr/local/tomcat/webapps/ROOT.war");

verifyMetrics(createMetricsVerifier());
}

private static MetricsVerifier createMetricsVerifier() {
AttributeMatcher requestProcessorNameAttribute =
attribute("tomcat.request_processor.name", "\"http-nio-8080\"");
AttributeMatcher threadPoolNameAttribute =
attribute("tomcat.thread_pool.name", "\"http-nio-8080\"");

return MetricsVerifier.create()
.add(
"tomcat.error.count",
metric ->
metric
.hasDescription("The number of errors.")
.hasUnit("{error}")
.isCounter()
.hasDataPointsWithOneAttribute(requestProcessorNameAttribute))
.add(
"tomcat.request.count",
metric ->
metric
.hasDescription("The number of requests processed.")
.hasUnit("{request}")
.isCounter()
.hasDataPointsWithOneAttribute(requestProcessorNameAttribute))
.add(
"tomcat.request.duration.max",
metric ->
metric
.hasDescription("The longest request processing time.")
.hasUnit("s")
.isGauge()
.hasDataPointsWithOneAttribute(requestProcessorNameAttribute))
.add(
"tomcat.request.processing_time",
metric ->
metric
.hasDescription("Total time for processing all requests.")
.hasUnit("s")
.isCounter()
.hasDataPointsWithOneAttribute(requestProcessorNameAttribute))
.add(
"tomcat.network.io",
metric ->
metric
.hasDescription("The number of bytes transmitted.")
.hasUnit("By")
.isCounter()
.hasDataPointsWithAttributes(
attributeGroup(
attribute("network.io.direction", "receive"),
requestProcessorNameAttribute),
attributeGroup(
attribute("network.io.direction", "transmit"),
requestProcessorNameAttribute)))
.add(
"tomcat.active_session.count",
metric ->
metric
.hasDescription("The number of active sessions.")
.hasUnit("{session}")
.isUpDownCounter()
.hasDataPointsWithOneAttribute(attributeWithAnyValue("tomcat.context")))
.add(
"tomcat.thread.count",
metric ->
metric
.hasDescription("Total thread count of the thread pool.")
.hasUnit("{thread}")
.isUpDownCounter()
.hasDataPointsWithOneAttribute(threadPoolNameAttribute))
.add(
"tomcat.thread.busy.count",
metric ->
metric
.hasDescription("Number of busy threads in the thread pool.")
.hasUnit("{thread}")
.isUpDownCounter()
.hasDataPointsWithOneAttribute(threadPoolNameAttribute));
}
}
14 changes: 14 additions & 0 deletions instrumentation/jmx-metrics/library/tomcat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Tomcat Metrics

Here is the list of metrics based on MBeans exposed by Tomcat.

| Metric Name | Type | Attributes | Description |
|--------------------------------|---------------|-----------------------------------------------------|---------------------------------------------|
| tomcat.active_session.count | UpDownCounter | tomcat.context | The number of active sessions. |
| tomcat.error.count | Counter | tomcat.request_processor.name | The number of errors. |
| tomcat.request.count | Counter | tomcat.request_processor.name | The number of requests processed. |
| tomcat.request.duration.max | Gauge | tomcat.request_processor.name | The longest request processing time. |
| tomcat.request.processing_time | Counter | tomcat.request_processor.name | Total time for processing all requests. |
| tomcat.network.io | Counter | tomcat.request_processor.name, network.io.direction | The number of bytes transmitted. |
| tomcat.thread.count | UpDownCounter | tomcat.thread_pool.name | Total thread count of the thread pool. |
| tomcat.thread.busy.count | UpDownCounter | tomcat.thread_pool.name | Number of busy threads in the thread pool. |
Loading