-
Notifications
You must be signed in to change notification settings - Fork 36
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
Export JVM metrics from the javaagent using micrometer #135
Conversation
README changes will be added in a separate PR
custom/src/main/java/com/splunk/opentelemetry/micrometer/OtelNamingConvention.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look really great. Just would like some unit tests for OtelNamingConvention
, but that could come in a separate PR if you would prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome stuff! Had a few ideas/comments, but looks good overall to me.
custom/src/main/java/com/splunk/opentelemetry/micrometer/MicrometerInstaller.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/MicrometerInstaller.java
Outdated
Show resolved
Hide resolved
bootstrap/src/main/java/com/splunk/opentelemetry/javaagent/bootstrap/GlobalMetricsTags.java
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/OtelNamingConvention.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/SplunkMetricsConfig.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/SplunkMetricsConfig.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/SplunkMetricsConfig.java
Outdated
Show resolved
Hide resolved
...ation/jvm-metrics/src/main/java/com/splunk/opentelemetry/jvmmetrics/JvmMetricsInstaller.java
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/SplunkMetricsConfig.java
Outdated
Show resolved
Hide resolved
...umentation/jvm-metrics/src/test/java/com/splunk/opentelemetry/jvmmetrics/JvmMetricsTest.java
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/OtelNamingConvention.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/com/splunk/opentelemetry/micrometer/MicrometerInstaller.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
Seems like the tests have passed (at least the linux ones...) -- @iNikem @kubawach @breedx-splk @jkwatson I've addressed all your comments, please re-review this PR 🙏 |
custom/src/test/java/com/splunk/opentelemetry/micrometer/OtelNamingConventionTest.java
Outdated
Show resolved
Hide resolved
custom/src/test/java/com/splunk/opentelemetry/micrometer/GlobalTagsBuilderTest.java
Show resolved
Hide resolved
12345L)); | ||
|
||
// when | ||
var tags = new GlobalTagsBuilder(resource).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java 11 in tests.. is there gradle magic that makes sure our code doesn't start using java 11 APIs that would break users on java 8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is:
compileJava {
options.release.set(8)
}
AFAIK this adds --release 8
when compiling src/main classes.
README changes will be added in a separate PR