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

[feat] - Optimize HTTP Client for Detectors #2805

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahrav
Copy link
Collaborator

@ahrav ahrav commented May 8, 2024

Description:

This PR introduces optimizations to the HTTP client used by detectors to improve connection reuse. It also updates all the detectors to utilize the new approach when setting up their HTTP clients. The main changes include:

  1. Added a new WithDetectorTransport function that returns a TransportOption. This function sets the desired transport settings for detectors, such as:
    • Increased Timeout to 5 seconds
    • Increased KeepAlive to 30 seconds
    • Increased MaxIdleConns to 100
    • Increased MaxIdleConnsPerHost to 10
    • Increased IdleConnTimeout to 90 seconds

I would appreciate feedback on these settings, as I'm not familiar with the ideal values. I found these online. 😅 They seemed ok.

  1. Modified the SaneHttpClient function to accept a variadic parameter opts of type ...TransportOption. This allows callers to pass in optional transport options.

  2. Updated the SaneHttpClient function to apply the provided transport options to the http.Transport before creating the http.Client.

  3. Updated all the detectors to use the new approach when setting up their HTTP clients. Instead of directly calling SaneHttpClient(), the detectors now call SaneHttpClient(WithDetectorTransport()) to create an HTTP client with the optimized transport settings specific to detectors.

Why:

During the analysis of CPU profiles, it was observed that a significant amount of time was being spent in the TLS handshake phase, specifically in the crypto/tls.(*Conn).clientHanshake function. This indicated that there was room for optimization in how the HTTP client handled TLS connections, particularly in scenarios with a high volume of concurrent requests, such as in the case of our detectors.

By introducing detector-specific transport settings and updating the detectors to use the optimized HTTP client, we aim to reduce the overhead associated with the TLS handshake. The increased timeouts, keep-alive duration, and maximum idle connections allow for better connection reuse and more efficient handling of concurrent requests.

These changes address the observed performance bottleneck in the TLS handshake and provide a more optimized HTTP client configuration for the specific needs of detectors while maintaining compatibility with the existing codebase.

Screenshot 2024-05-07 at 6 18 36 PM

Checklist:

  • Tests passing (make test-community)?
  • Lint passing (make lint this requires golangci-lint)?

@rgmz
Copy link
Contributor

rgmz commented May 8, 2024

I think #2086 would also be needed for optimal connection reuse.

Edit: In retrospect, I would create a reusable function in common rather than adding a large multi-line defer statement to each detector.

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

Successfully merging this pull request may close these issues.

2 participants