-
Notifications
You must be signed in to change notification settings - Fork 88
[MMM-20372] Make pool_maxsize configurable. #1642
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
base: master
Are you sure you want to change the base?
[MMM-20372] Make pool_maxsize configurable. #1642
Conversation
@@ -225,8 +227,18 @@ def setup_otel(runtime_parameters, options): | |||
# (most frequent case) | |||
multiprocessing = options.max_workers > 1 | |||
|
|||
pool_maxsize = 30 # reqeusts default is 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
total_concurrent_requests = 500
num_workers = self._params.get("processes")
pool_maxsize = ceil(total_concurrent_requests / num_workers)
for 8 cores it is ~60
pool_maxsize = int(runtime_parameters.get("DR_OTEL_SESSION_POOL_MAXSIZE")) | ||
|
||
session = requests.Session() | ||
adapter = requests.adapters.HTTPAdapter(pool_maxsize=pool_maxsize) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Custom adapter with default timeout
class TimeoutHTTPAdapter(requests.adapters.HTTPAdapter):
def __init__(self, *args, timeout=5, **kwargs):
self.timeout = timeout
super().__init__(*args, **kwargs)
def send(self, request, **kwargs):
kwargs["timeout"] = kwargs.get("timeout", self.timeout)
return super().send(request, **kwargs)
adapter = TimeoutHTTPAdapter(pool_maxsize=pool_maxsize, timeout=5)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably need timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or
class TimeoutSession(requests.Session):
def __init__(self, timeout=(5, 15)):
super().__init__()
self._timeout = timeout
def request(self, *args, **kwargs):
kwargs.setdefault("timeout", self._timeout)
return super().request(*args, **kwargs)
# Example usage:
pool_maxsize = 10 # or any desired value
session = TimeoutSession(timeout=(5, 15))
adapter = HTTPAdapter(pool_maxsize=pool_maxsize)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exporter already has timeout config that they pass into session, can be configured by env var:
OTEL_EXPORTER_OTLP_TRACES_TIMEOUT
see
https://github.com/open-telemetry/opentelemetry-python/blob/31289bd1722fcb8b09d9b7a36a7e00d5cf7e17a3/exporter/opentelemetry-exporter-otlp-proto-http/src/opentelemetry/exporter/otlp/proto/http/trace_exporter/__init__.py#L111-L115
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default timeout is 10 sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to check timeouts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to check timeout.
Should address:
in high perf scenarios.