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
Register and schedule Micrometer MeterRegistry when Meters are registered #2255
Register and schedule Micrometer MeterRegistry when Meters are registered #2255
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
...crometer-plugin/src/main/java/co/elastic/apm/agent/micrometer/MicrometerInstrumentation.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.
LGTM, should be good as it has been validated in the field.
@@ -368,7 +368,6 @@ void testExclusionOfFailedGauge_singleGauge() { | |||
.isEmpty(); | |||
|
|||
getMetricSets(); | |||
assertThat(metricsReporter.getFailedMeters().iterator().next().getId().getName()).isEqualTo("gauge1"); |
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.
[minor] Was this change required due to upgrading micrometer that have different behavior between 1.0.1 and 1.8.0 or due to the change of instrumented method ?
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.
It's a different behavior - in the new version the abstract meters capture exceptions quietly, so we don't get to see that they failed
What does this PR do?
As reported in spring-projects/spring-boot#26630, premature initialization of Micrometer's
MeterRegistry
in Spring applications may cause metrics loss.This PR attempts to work around this issue assuming that
Meter
s are only registered to fully initialized meter registries.This assumption is yet to be verified.
Checklist
I have added tests that would fail without this fixdependabot.yml
so that we continuously test against the latest versionMicrometerInstrumentationVersionsIT
to test with version 1.0.1