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 22 commits into
base: main
Choose a base branch
from

Conversation

robsunday
Copy link
Contributor

@robsunday robsunday commented Apr 4, 2025

Changes:

  1. Metric names made semconv aligned
  2. Attribute names made semconv aligned
  3. Metrics descriptions format standardized
  4. tomcat.yaml moved from agent to library folder to make it reusable in all library dependants
  5. Tomcat JMX metrics Integration test implemented. It utilize assertions and target system integration test framework ported by @SylvainJuge from opentelemetry-java-contrib

Tomcat metrics description md file moved to library
Code review followup
@@ -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

@robsunday robsunday marked this pull request as ready for review April 8, 2025 13:10
@robsunday robsunday requested a review from a team as a code owner April 8, 2025 13:10
@SylvainJuge
Copy link
Contributor

@robsunday #13597 has been merged, so you can update with current state of main to make diff simpler.

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.

Copy link
Contributor Author

@robsunday robsunday May 14, 2025

Choose a reason for hiding this comment

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

I mapped maxTime to tomcat.request.duration.max to make it similar to one of http semconv metrics
I'd keep this name, and i could then map processingTime to tomcat.request.duration.total
I think .total suffix is fine because semconv rule you mentioned is talking specifically about _total suffix, not .total

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worth to ask and double-check in the SIG meeting if .total could be fine as suffix, because if it does then it sounds like a good strategy that would be consistent with other "pre-aggregated metrics", for example it's easy to add an tomcat.request.duration.max

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.

It would be worth to ask and double-check in the SIG meeting if .total could be fine as suffix, because if it does then it sounds like a good strategy that would be consistent with other "pre-aggregated metrics", for example it's easy to add an tomcat.request.duration.max

- Tomcat:type=GlobalRequestProcessor,name=*
prefix: tomcat.
metricAttribute:
tomcat.request_processor.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.

For the processor name, I think we could maybe use tomcat.request.processor.name as it allows to reuse the tomcat.request namespace, and as I understand defining more namespaces than strictly needed in semconv and avoiding _ is the current strategy, even if we don't have anything extra to provide in the tomcat.request.processor name yet.

tomcat.request.processing_time changed to tomcat.request.duration.total
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