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

remove metrics from MeterRegistry when shutting down InstrumentedQueuedThreadPool #4003

Merged

Conversation

wakingrufus
Copy link
Contributor

@wakingrufus wakingrufus commented Jul 27, 2023

remove metrics from MeterRegistry when shutting down JettyServerThreadPoolMetrics/InstrumentedQueuedThreadPool

fixes #4000

Rationale

Clean up resources when not needed anymore

Additional context

The dropwizard metrics jetty InstrumentedQueuedThreadPool does something similar, and it seems like a good idea to do here as well.

@wakingrufus wakingrufus changed the title …remove metrics from MeterRegistry when shutting down InstrumentedQueuedThreadPool remove metrics from MeterRegistry when shutting down InstrumentedQueuedThreadPool Jul 27, 2023
@wakingrufus wakingrufus force-pushed the cleanup-jetty-metrics branch from 1351b4d to 95fff2b Compare July 27, 2023 15:01
@wakingrufus
Copy link
Contributor Author

Hi @shakuzen would you mind taking a look at this PR? Thanks

@wakingrufus
Copy link
Contributor Author

@jonatan-ivanov I would really appreciate if someone could take a look at this PR. Thanks!
Sorry for the direct ping, but if I am going to ping someone random, I figured it might as well be someone who I have played duck bowling with ;)

Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to review, and thanks for opening the pull request.

@wakingrufus wakingrufus force-pushed the cleanup-jetty-metrics branch from 95fff2b to 17f3c8f Compare February 5, 2024 15:13
@wakingrufus wakingrufus requested a review from shakuzen February 5, 2024 15:15
@wakingrufus wakingrufus force-pushed the cleanup-jetty-metrics branch from 17f3c8f to cd06911 Compare February 5, 2024 15:19
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

Thank you for quickly updating the pull request. I've left another review.

@wakingrufus wakingrufus force-pushed the cleanup-jetty-metrics branch 2 times, most recently from a13d34b to 1c3c2ed Compare February 6, 2024 15:28
@wakingrufus wakingrufus requested a review from shakuzen February 6, 2024 15:44
@wakingrufus wakingrufus force-pushed the cleanup-jetty-metrics branch from 1c3c2ed to 0e9977c Compare February 7, 2024 23:26
@wakingrufus
Copy link
Contributor Author

I have made all requested changes. Please take another look.
Thanks again!

@shakuzen shakuzen merged commit ce60b13 into micrometer-metrics:main Feb 9, 2024
6 checks passed
@shakuzen
Copy link
Member

shakuzen commented Feb 9, 2024

Thank you for the pull request and patience and all the quick turnarounds on feedback.

@wakingrufus wakingrufus deleted the cleanup-jetty-metrics branch February 9, 2024 15:00
izeye added a commit to izeye/micrometer that referenced this pull request Apr 18, 2024
@izeye izeye mentioned this pull request Apr 18, 2024
shakuzen pushed a commit that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove metrics when shutting down InstrumentedQueuedThreadPool
3 participants