Skip to content

Commit

Permalink
fix(bot): http client perf issue (#852)
Browse files Browse the repository at this point in the history
Work around for a perf issue with how httpx handles SSL.

Instead of using `httpx.AsyncClient` directly, we subclass and reuse the ssl context.

rel: encode/httpx#838
  • Loading branch information
sbdchd authored Oct 25, 2022
1 parent 7127cdf commit a11f216
Show file tree
Hide file tree
Showing 10 changed files with 66 additions and 12 deletions.
4 changes: 2 additions & 2 deletions bot/kodiak/cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@

import click
import requests
from httpx import AsyncClient

from kodiak import app_config as conf
from kodiak.config import V1
from kodiak.http import HttpClient
from kodiak.queries import generate_jwt, get_token_for_install


Expand Down Expand Up @@ -66,7 +66,7 @@ def token_for_install(install_id: str) -> None:
"""

async def get_token() -> str:
async with AsyncClient() as http:
async with HttpClient() as http:
return await get_token_for_install(session=http, installation_id=install_id)

token = asyncio.run(get_token())
Expand Down
36 changes: 36 additions & 0 deletions bot/kodiak/http.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from __future__ import annotations

import ssl

from httpx import ( # noqa: I251
AsyncClient,
HTTPError,
HTTPStatusError,
Request,
Response,
)
from httpx._config import DEFAULT_TIMEOUT_CONFIG # noqa: I251
from httpx._types import TimeoutTypes # noqa: I251

__all__ = ["Response", "Request", "HTTPError", "HttpClient", "HTTPStatusError"]

# NOTE: this has a cost to create so we may want to set this lazily on the first HttpClient creation
context = ssl.create_default_context()


class HttpClient(AsyncClient):
"""
HTTP Client with the SSL config cached at the module level to avoid perf issues.
see: https://github.com/encode/httpx/issues/838
"""

def __init__(
self,
*,
timeout: TimeoutTypes = DEFAULT_TIMEOUT_CONFIG,
):

super().__init__(
verify=context,
timeout=timeout,
)
2 changes: 1 addition & 1 deletion bot/kodiak/pull_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from typing import Awaitable, Callable, List, Optional, Type

import structlog
from httpx import HTTPStatusError as HTTPError
from typing_extensions import Protocol

import kodiak.app_config as conf
Expand All @@ -16,6 +15,7 @@
RetryForSkippableChecks,
)
from kodiak.evaluation import mergeable
from kodiak.http import HTTPStatusError as HTTPError
from kodiak.queries import Client, EventInfoResponse

logger = structlog.get_logger()
Expand Down
5 changes: 3 additions & 2 deletions bot/kodiak/queries/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from enum import Enum
from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Union, cast

import httpx as http
import jwt
import pydantic
import structlog
Expand All @@ -16,7 +15,9 @@
from typing_extensions import Literal, Protocol

import kodiak.app_config as conf
from kodiak import http
from kodiak.config import V1, MergeMethod
from kodiak.http import HttpClient
from kodiak.queries.commits import Commit, CommitConnection, GitActor
from kodiak.queries.commits import User as PullRequestCommitUser
from kodiak.queries.commits import get_commits
Expand Down Expand Up @@ -825,7 +826,7 @@ def __init__(self, *, owner: str, repo: str, installation_id: str):
self.installation_id = installation_id
# NOTE: We must call `await session.close()` when we are finished with our session.
# We implement an async context manager this handle this.
self.session = http.AsyncClient(
self.session = HttpClient(
# infinite timeout to match behavior of old, requests_async http
# client. As a backup we have an asyncio timeout of 30 seconds.
timeout=None
Expand Down
6 changes: 3 additions & 3 deletions bot/kodiak/refresh_pull_requests.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,12 @@
import structlog
from asyncio_redis.connection import Connection as RedisConnection
from asyncio_redis.replies import BlockingPopReply
from httpx import AsyncClient
from pydantic import BaseModel
from sentry_sdk.integrations.logging import LoggingIntegration

from kodiak import app_config as conf
from kodiak import redis_client
from kodiak.http import HttpClient
from kodiak.logging import SentryProcessor, add_request_info_processor
from kodiak.queries import generate_jwt, get_token_for_install
from kodiak.queue import WebhookEvent
Expand Down Expand Up @@ -106,7 +106,7 @@
"""


async def get_login_for_install(*, http: AsyncClient, installation_id: str) -> str:
async def get_login_for_install(*, http: HttpClient, installation_id: str) -> str:
app_token = generate_jwt(
private_key=conf.PRIVATE_KEY, app_identifier=conf.GITHUB_APP_ID
)
Expand All @@ -124,7 +124,7 @@ async def get_login_for_install(*, http: AsyncClient, installation_id: str) -> s
async def refresh_pull_requests_for_installation(
*, installation_id: str, redis: RedisConnection
) -> None:
async with AsyncClient() as http:
async with HttpClient() as http:
login = await get_login_for_install(http=http, installation_id=installation_id)
token = await get_token_for_install(
session=http, installation_id=installation_id
Expand Down
4 changes: 2 additions & 2 deletions bot/kodiak/test_pull_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

from typing import Any, Awaitable, Callable, Dict, List, Mapping, Optional, Type, cast

import httpx as requests
import pytest
from httpx import Request
from typing_extensions import Protocol

import kodiak.http as requests
from kodiak.config import V1, Merge, MergeMethod
from kodiak.errors import ApiCallException
from kodiak.http import Request
from kodiak.pull_request import PRV2, EventInfoResponse, QueueForMergeCallback
from kodiak.queries import (
BranchProtectionRule,
Expand Down
2 changes: 1 addition & 1 deletion bot/kodiak/test_queries.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@

import asyncio_redis
import pytest
from httpx import Request, Response
from pytest_mock import MockFixture

from kodiak import app_config as conf
from kodiak.config import V1, Merge, MergeMethod
from kodiak.http import Request, Response
from kodiak.queries import (
BranchProtectionRule,
CheckConclusionState,
Expand Down
15 changes: 14 additions & 1 deletion bot/poetry.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions bot/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pytest-cov = "^2.10"
flake8-pyi = "^20.10"
types-requests = "^2.28.0"
types-toml = "^0.10.7"
flake8-tidy-imports = "^4.8.0"

[tool.poetry.plugins."pytest11"]
"pytest_plugin" = "pytest_plugin.plugin"
Expand Down
3 changes: 3 additions & 0 deletions bot/tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ filterwarnings =
ignore::pytest.PytestDeprecationWarning
ignore::DeprecationWarning:asyncio_redis
[flake8]
banned-modules =
httpx.* = Use kodiak.http
ban-relative-imports = true
ignore =
; formatting handled by black
; https://pycodestyle.readthedocs.io/en/latest/intro.html#error-codes
Expand Down

0 comments on commit a11f216

Please sign in to comment.