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

JMX Metrics to OTLP & Breeze #3406

Merged
merged 44 commits into from Jan 25, 2024
Merged

Conversation

harsimar
Copy link
Contributor

@harsimar harsimar commented Nov 28, 2023

This PR contains changes for customers to collect JMX metrics via OTLP and application insights. It expects that the customer sets the environment variable or system property for the otel metrics exporter to use OTLP when launching their own application. However, application insights will set the otel.metric.export.interval to match metricIntervalSeconds from applicationinsights.json. An example launch of the spring pet clinic sample app below:

java -javaagent:"<path to app insights java agent>" -Dotel.metrics.exporter=otlp -jar target\spring-petclinic-3.1.0-SNAPSHOT.jar

A customer could then set up an OTLP collector/endpoint to receive metrics.

A few notes about behavior:

Test coverage:

  • Added more test cases to JmxMetricTest.java
  • Modified test cases for JmxDataFetcherTest.java
  • manually ran spring pet clinic java app with application insights java agent attached & otlp exporter/collector set to verify behavior.

Comment on lines 118 to 122
} catch (RuntimeException e) {
try (MDC.MDCCloseable ignored = CUSTOM_JMX_METRIC_ERROR.makeActive()) {
logger.error("Failed to register JMX performance counters: '{}'", e.toString());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

it looks like previously we had try/catch around each JMX metric, which could be better since then a single JMX metric problem wouldn't affect the others

Copy link
Contributor

Choose a reason for hiding this comment

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

there is another try block above.

@trask trask merged commit 1deaee2 into microsoft:main Jan 25, 2024
85 checks passed
@trask
Copy link
Member

trask commented Jan 25, 2024

thx @harsimar @heyams!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants