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

Fix Micrometer registry serialization error due to failed meter #1741

Merged

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Apr 4, 2021

What does this PR do?

When a Micrometer meter throws Exception while trying to retrieve some meter value, the serialization of the entire metric registry will fail. If the failure is consistent, all meters of this registry would never be reported.
With this PR, a meter can fail only once, omitting a single warning, without failing the serialization of the registry.

In addition, in an attempt to fix at least one type of errors related to querying meter values, I tried to set the meter's class loader as the thread's context CL for the time of query.
NOTE FOR REVIEWER I think it's lightweight enough to do for each meter, but we can also have two caches of failed meters, so that we don't set context CL by default, try to re-execute meter query after the first failure with the context CL and if succeeds, add it to a different cache. If the second attempt fails, we can then add to the generally-disabled meters.

Checklist

  • This is a bugfix

@eyalkoren eyalkoren added the bug Bugs label Apr 4, 2021
@elastic-apm-tech elastic-apm-tech added this to In Progress in APM-Agents (OLD) Apr 4, 2021
@apmmachine
Copy link
Collaborator

apmmachine commented Apr 4, 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #1741 updated

  • Start Time: 2021-04-19T08:03:40.797+0000

  • Duration: 59 min 39 sec

  • Commit: 70012cb

Test stats 🧪

Test Results
Failed 0
Passed 1989
Skipped 19
Total 2008

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 1989
Skipped 19
Total 2008

@SylvainJuge SylvainJuge merged commit cfe60e1 into elastic:master Apr 19, 2021
APM-Agents (OLD) automation moved this from In Progress to Done Apr 19, 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

3 participants