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

Use InstrumentedQueuedThreadPool for admin endpoint #2186

Merged
merged 1 commit into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@patrox
Contributor

patrox commented Oct 26, 2017

This will allow to monitor the usage of the thread pool which is used to handle the requests landing at admin endpoint.

Closes #2179.

@coveralls

This comment has been minimized.

coveralls commented Oct 26, 2017

Coverage Status

Coverage remained the same at 85.484% when pulling 98ee58a on patrox:issue_2179_instrumented_admin_thread_pool into 9b35878 on dropwizard:master.

@patrox

This comment has been minimized.

Contributor

patrox commented Oct 26, 2017

@joschi Can you please have a look ?

@isaki

This comment has been minimized.

Contributor

isaki commented Oct 26, 2017

Is there a way to make this optional/configurable?

@patrox

This comment has been minimized.

Contributor

patrox commented Oct 26, 2017

To be honest I didn't consider it, as I haven't seen any downsides of using it, but I guess it could be made configurable by some additional boolean flag, i.e. isAdminThreadPoolMonitored.

@arteam

This comment has been minimized.

Member

arteam commented Oct 26, 2017

I don't think it's needed. The overhead of instrumenting should be fairly small to warrant adding an additional configuration option.

@arteam

This comment has been minimized.

Member

arteam commented Oct 26, 2017

The biggest concern for me is the increase of reported metrics by default for a Dropwizard app.

@arteam

This comment has been minimized.

Member

arteam commented Oct 26, 2017

thread-pools

@arteam

This comment has been minimized.

Member

arteam commented Oct 26, 2017

But, my opinion, 4 additional metrics should not be a big problem. They are quite useful metrics, especially for applications with a lot of admin tasks.

@arteam arteam merged commit d66e51f into dropwizard:master Oct 26, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 85.484%
Details
@arteam

This comment has been minimized.

Member

arteam commented Oct 26, 2017

@patrox Thank you for the contribution!

@arteam arteam added this to the 1.2.1 milestone Oct 26, 2017

@arteam arteam added the improvement label Oct 26, 2017

sankate pushed a commit to sankate/dropwizard that referenced this pull request Nov 21, 2017

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment