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

Register and schedule Micrometer MeterRegistry when Meters are registered #2255

Merged
merged 3 commits into from Nov 24, 2021

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Nov 11, 2021

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 Meters are only registered to fully initialized meter registries.
This assumption is yet to be verified.

Checklist

  • This is a bugfix
    • I have updated CHANGELOG.asciidoc
    • I have added tests that would fail without this fix
    • I verified the described issue is resolved using this fix where it is reproducible without this fix
    • I upgraded Micrometer dependency to latest and updated dependabot.yml so that we continuously test against the latest version
    • I added a MicrometerInstrumentationVersionsIT to test with version 1.0.1

@apmmachine
Copy link
Collaborator

apmmachine commented Nov 11, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-23T12:39:17.748+0000

  • Duration: 74 min 39 sec

  • Commit: fec7104

Test stats 🧪

Test Results
Failed 0
Passed 2546
Skipped 21
Total 2567

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run compatibility tests : Run the JDK Compatibility test.

  • run integration tests : Run the APM-ITs.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@eyalkoren eyalkoren requested review from felixbarny and SylvainJuge and removed request for felixbarny November 21, 2021 12:56
Copy link
Member

@SylvainJuge SylvainJuge left a 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");
Copy link
Member

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 ?

Copy link
Contributor Author

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

@SylvainJuge SylvainJuge added the await-release Mark issues that depend on next release, or PRs that are planned to be included label Nov 24, 2021
@eyalkoren eyalkoren merged commit 2f5ede2 into elastic:master Nov 24, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Nov 24, 2021
@SylvainJuge SylvainJuge removed the await-release Mark issues that depend on next release, or PRs that are planned to be included label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants