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

Apache connection pool instrumentation #457

Closed
wants to merge 5 commits into from

Conversation

iamdanfox
Copy link
Contributor

Before this PR

We have hypotheses that connection re-use has a big impact on our most performance-sensitive services like timelock. Currently, we just use the tls.handshake graphs to

After this PR

==COMMIT_MSG==
Each closeable apache client now exposes methods which reveal statistics about the connection pool.
==COMMIT_MSG==

After merging, we'll have to carefully wire these up to gauges in witchcraft, ensuring that we don't prevent objects from being GC'd just by having a reference in a tritium registry somewhere.

Possible downsides?

  • if the apache defaults change then by building our connection manager ourself we might miss out on improvements

@changelog-app
Copy link

changelog-app bot commented Feb 28, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Each closeable apache client now exposes methods which reveal statistics about the connection pool.

Check the box to generate changelog(s)

  • Generate changelog entry

assertThat(client.getNumRoutes()).describedAs("routes").isEqualTo(1);
assertThat(client.getPoolStats().getAvailable())
.describedAs("available")
.isEqualTo(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think graphing the number available will be kinda interesting, as this should tell us how quickly we could handle bursts of traffic

 /**
     * Gets the number idle persistent connections.
     * <p>
     * The total number of connections in the pool is equal to {@code available} plus {@code leased}.
     * </p>
     *
     * @return number idle persistent connections.
     */
    public int getAvailable() {

PoolingHttpClientConnectionManager connectionManager = new PoolingHttpClientConnectionManager(
RegistryBuilder.<ConnectionSocketFactory>create()
.register("http", PlainConnectionSocketFactory.getSocketFactory())
.register("https", sslSocketFactory)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can wrap these to add a meter of connection rate, and wrap the pool manager to capture the rate connections requested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmm that would be cool, so dividing one by the other tells us how effectively we're re-using... Do we not already get that connection rate from the tritium tls.handshakes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tritium handshake instrumentation would work assuming https, but I don’t think we wire it up yet.

@ferozco
Copy link
Contributor

ferozco commented Mar 6, 2020

Closing in favour of #498

@ferozco ferozco closed this Mar 6, 2020
@iamdanfox iamdanfox deleted the dfox/connection-pool-instrumentation branch April 27, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants