Changed: config #14
Changed: config #14
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP connection pool configuration for OTLP (OpenTelemetry Protocol) exporters in the Hyperf OpenTelemetry library. The configuration enables connection pooling for improved performance when sending telemetry data to OTLP collectors.
Changes:
- Added
otlp_http.poolconfiguration section with connection pool settings (min/max connections and timeout values)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'min_connections' => 3, | ||
| 'max_connections' => 30, | ||
| 'connect_timeout' => 0.1, | ||
| 'wait_timeout' => 0.1, |
There was a problem hiding this comment.
The pool configuration values are hardcoded, which is inconsistent with the rest of the configuration file. All other configuration values in this file use env() to allow environment-based configuration (e.g., env('OTEL_TRACES_ENDPOINT', ...) on line 43, env('OTEL_METRICS_ENDPOINT', ...) on line 90).
Consider making these values configurable via environment variables to maintain consistency and allow per-environment customization. For example:
- 'min_connections' => (int) env('OTEL_OTLP_HTTP_POOL_MIN_CONNECTIONS', 3)
- 'max_connections' => (int) env('OTEL_OTLP_HTTP_POOL_MAX_CONNECTIONS', 30)
- 'connect_timeout' => (float) env('OTEL_OTLP_HTTP_POOL_CONNECT_TIMEOUT', 0.1)
- 'wait_timeout' => (float) env('OTEL_OTLP_HTTP_POOL_WAIT_TIMEOUT', 0.1)
|
|
||
| 'otlp_http' => [ | ||
| 'pool' => [ | ||
| 'min_connections' => 3, | ||
| 'max_connections' => 30, | ||
| 'connect_timeout' => 0.1, |
There was a problem hiding this comment.
The new 'otlp_http' pool configuration section lacks documentation explaining its purpose and when it should be used. Other complex configuration sections in this file (such as the traces, metrics, and logs sections) have clear structure with nested arrays that make their purpose evident.
Consider adding inline comments to explain what this pool configuration controls (HTTP connection pooling for OTLP exporters) and how it affects the application's behavior. This would help users understand when and why they might need to adjust these values.
| 'otlp_http' => [ | |
| 'pool' => [ | |
| 'min_connections' => 3, | |
| 'max_connections' => 30, | |
| 'connect_timeout' => 0.1, | |
| // Global HTTP connection pool configuration used by OTLP HTTP exporters (traces, metrics, logs). | |
| // Adjust these values if you see connection saturation or need to tune resource usage | |
| // for high-throughput telemetry export. | |
| 'otlp_http' => [ | |
| 'pool' => [ | |
| // Minimum number of idle HTTP connections to keep in the pool per worker. | |
| 'min_connections' => 3, | |
| // Maximum number of HTTP connections that can be opened per worker. | |
| 'max_connections' => 30, | |
| // Timeout (in seconds) for establishing a new HTTP connection to the OTLP endpoint. | |
| 'connect_timeout' => 0.1, | |
| // Maximum time (in seconds) to wait for a free connection from the pool | |
| // before failing the export attempt. |
| 'otlp_http' => [ | ||
| 'pool' => [ | ||
| 'min_connections' => 3, | ||
| 'max_connections' => 30, | ||
| 'connect_timeout' => 0.1, | ||
| 'wait_timeout' => 0.1, | ||
| ], | ||
| ], |
There was a problem hiding this comment.
The PR description template has not been filled out. The description should explain:
- What this configuration change does (adds HTTP connection pool settings for OTLP exporters)
- Why it's needed (performance optimization, resource management, etc.)
- The type of change (appears to be a configuration enhancement)
- Whether tests were added/updated
- Whether documentation needs updating (README doesn't currently mention pool configuration)
A complete PR description helps reviewers understand the context and rationale for changes.
| 'connect_timeout' => 0.1, | ||
| 'wait_timeout' => 0.1, |
There was a problem hiding this comment.
The connection timeout values (0.1 seconds for both connect_timeout and wait_timeout) are extremely short and may cause connection failures in production environments, especially with network latency or when connecting to remote OTLP collectors.
For comparison, other timeouts in this configuration file use more generous defaults (e.g., timeout of 3 seconds on line 47 for traces). Consider whether these aggressive timeout values are appropriate for your use case. Timeouts this short (100ms) are typically only suitable for local or same-datacenter connections with very low latency.
2 participants
Description
Explain what this PR does and why it is needed.
Type of change
Checklist
feat: add SQS aspect)composer testpasses locallyRelated issues
Closes #, Relates to #