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 when shutting down InstrumentedQueuedThreadPool #4000

Closed
wakingrufus opened this issue Jul 26, 2023 · 4 comments · Fixed by #4003
Closed

Remove metrics when shutting down InstrumentedQueuedThreadPool #4000

wakingrufus opened this issue Jul 26, 2023 · 4 comments · Fixed by #4003
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Milestone

Comments

@wakingrufus
Copy link
Contributor

wakingrufus commented Jul 26, 2023

remove metrics from MeterRegistry when shutting down InstrumentedQueuedThreadPool
Something like:

    @Override
    protected void doStop() throws Exception {
       Set<Tag> tagSet = new HashSet<>();
        tags.forEach(tagSet::add);
        Stream.of("jetty.threads.config.min",
                        "jetty.threads.config.max",
                        "jetty.threads.busy",
                        "jetty.threads.jobs",
                        "jetty.threads.current",
                        "jetty.threads.idle")
                .flatMap(name -> meterRegistry.find(name).meters().stream())
                .map(Meter::getId)
                .filter(id -> new HashSet<>(id.getTags()).containsAll(tagSet))
                .forEach(meterRegistry::remove);
        super.doStop();
    }

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.

I am happy to open a PR for this with tests if it is agreed that this should exist.

Thanks

@shakuzen
Copy link
Member

It sounds like it may make sense, but I'd like to understand more. In most cases, I would expect stopping the threadpool to happen on shutdown of the Server which would also correspond to shutting down the JVM. In that case, I don't think it matters much if we remove the meters. Is this proposal for cases where Jetty Servers will be started and stopped multiple times while the JVM is running?

@shakuzen shakuzen added waiting for feedback We need additional information before we can continue and removed waiting-for-triage labels Jul 27, 2023
@wakingrufus
Copy link
Contributor Author

Yes. Another place where it may be nice is with running multiple integration tests, spinning up and shutting down Jetty multiple times for different tests, but all using the same globalRegistry. This would provide better isolation for such tests.

@shakuzen
Copy link
Member

We offer a clear method for cleaning up the registered meters on a global registry. Though we generally advise against using a global registry as much as possible to avoid having the issue in the first place. Nonetheless, it wouldn't be a bad idea for us to clean up as you suggest. Please do send us a pull request.

@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module and removed waiting for feedback We need additional information before we can continue labels Jul 27, 2023
@shakuzen shakuzen added this to the 1.x milestone Jul 27, 2023
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Jul 27, 2023
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Jul 27, 2023
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Jul 27, 2023
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Jul 27, 2023
@wakingrufus
Copy link
Contributor Author

Ok I have opened a PR for this. please take a look and let me know if there is anything I can improve.

Thanks again!

@shakuzen shakuzen changed the title remove metrics from MeterRegistry when shutting down InstrumentedQueuedThreadPool Remove metrics when shutting down InstrumentedQueuedThreadPool Feb 5, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 5, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 5, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 6, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 6, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 7, 2024
wakingrufus added a commit to wakingrufus/micrometer that referenced this issue Feb 8, 2024
shakuzen pushed a commit that referenced this issue Feb 9, 2024
…mentedQueuedThreadPool (#4003)

Jetty servers may be stopped and started. The metrics from stopped servers will not be updated anymore, so they can be removed. Keeps track of registered metrics in JettyServerThreadPoolMetrics and removes them on `close`.

fixes gh-4000
@shakuzen shakuzen modified the milestones: 1.x, 1.13.0-M1 Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement module: micrometer-core An issue that is related to our core module
Projects
None yet
2 participants