feat(otlphttp): configure response body size limit - #8443
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8443 +/- ##
=====================================
Coverage 83.9% 83.9%
=====================================
Files 323 323
Lines 25814 25867 +53
=====================================
+ Hits 21676 21724 +48
+ Misses 3758 3757 -1
- Partials 380 386 +6
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a configurable response body size limit for the OTLP/HTTP trace, metric, and log exporters, preserving the existing 4 MiB default while allowing users to tune it (including disabling the limit with size <= 0) in line with OTLP/HTTP spec guidance.
Changes:
- Introduces
WithMaxResponseBodySize(int64)in OTLP/HTTP trace/metric/log exporters and wires it through each exporter’s HTTP config. - Replaces the prior package-level response size limit variables with per-client config and a shared
copyResponseBodyhelper to enforce/disable the limit. - Updates response-body-size-limit tests to use the new option and documents the new options in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 17 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/shared/otlp/otlptrace/otlpconfig/options.go.tmpl | Adds HTTP-only default/config field + option for max response body size in shared trace config template. |
| internal/shared/otlp/otlpmetric/oconf/options.go.tmpl | Adds HTTP-only default/config field + option for max response body size in shared metric config template. |
| exporters/otlp/otlptrace/otlptracehttp/options.go | Exposes WithMaxResponseBodySize as a public trace HTTP exporter option. |
| exporters/otlp/otlptrace/otlptracehttp/internal/otlpconfig/options.go | Generated trace HTTP config now includes MaxResponseBodySize default + option plumbing. |
| exporters/otlp/otlptrace/otlptracehttp/internal/gen.go | Passes isHTTP to templates to conditionally generate HTTP-only settings. |
| exporters/otlp/otlptrace/otlptracehttp/export_test.go | Removes prior test-only export of the package-level response limit override. |
| exporters/otlp/otlptrace/otlptracehttp/client.go | Enforces response body size via config (MaxResponseBodySize) and adds copyResponseBody helper with disable support. |
| exporters/otlp/otlptrace/otlptracehttp/client_test.go | Updates response body size limit test to use WithMaxResponseBodySize(1) instead of global override. |
| exporters/otlp/otlptrace/otlptracegrpc/internal/otlpconfig/options.go | Regenerates gRPC trace config from templates (no response-size setting for non-HTTP). |
| exporters/otlp/otlptrace/otlptracegrpc/internal/gen.go | Passes isHTTP: false to templates for gRPC trace exporter generation. |
| exporters/otlp/otlpmetric/otlpmetrichttp/internal/oconf/options.go | Generated metric HTTP config now includes MaxResponseBodySize default + option plumbing. |
| exporters/otlp/otlpmetric/otlpmetrichttp/internal/gen.go | Passes isHTTP to metric templates to conditionally generate HTTP-only settings. |
| exporters/otlp/otlpmetric/otlpmetrichttp/config.go | Exposes WithMaxResponseBodySize as a public metric HTTP exporter option. |
| exporters/otlp/otlpmetric/otlpmetrichttp/client.go | Enforces response body size via config and adds copyResponseBody helper with disable support. |
| exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go | Updates response body size limit test to use WithMaxResponseBodySize(1) instead of global override. |
| exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf/options.go | Regenerates gRPC metric config from templates (no response-size setting for non-HTTP). |
| exporters/otlp/otlpmetric/otlpmetricgrpc/internal/gen.go | Passes isHTTP: false to templates for gRPC metric exporter generation. |
| exporters/otlp/otlplog/otlploghttp/config.go | Adds log HTTP exporter config setting + WithMaxResponseBodySize option (non-templated config). |
| exporters/otlp/otlplog/otlploghttp/client.go | Enforces response body size via config and adds copyResponseBody helper with disable support. |
| exporters/otlp/otlplog/otlploghttp/client_test.go | Updates response body size limit test to use WithMaxResponseBodySize(1) instead of global override. |
| CHANGELOG.md | Documents the newly added WithMaxResponseBodySize options for trace/metric/log HTTP exporters. |
Files not reviewed (4)
- exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf/options.go: Language not supported
- exporters/otlp/otlpmetric/otlpmetrichttp/internal/oconf/options.go: Language not supported
- exporters/otlp/otlptrace/otlptracegrpc/internal/otlpconfig/options.go: Language not supported
- exporters/otlp/otlptrace/otlptracehttp/internal/otlpconfig/options.go: Language not supported
|
@AbhiPrasad, are you able to address comments or do you want me to take over? |
sorry for the delay, my day job got in the way 😄 Pushed up commits to address the review changes! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 21 changed files in this pull request and generated 6 comments.
Files not reviewed (4)
- exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf/options.go: Generated file
- exporters/otlp/otlpmetric/otlpmetrichttp/internal/oconf/options.go: Generated file
- exporters/otlp/otlptrace/otlptracegrpc/internal/otlpconfig/options.go: Generated file
- exporters/otlp/otlptrace/otlptracehttp/internal/otlpconfig/options.go: Generated file
|
@AbhiPrasad, I agree with Copilot 😉 Can you address the comments when you find some time? |
|
I agree as well! pushed up the fix with cf9968e |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 21 changed files in this pull request and generated 5 comments.
Files not reviewed (4)
- exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf/options.go: Generated file
- exporters/otlp/otlpmetric/otlpmetrichttp/internal/oconf/options.go: Generated file
- exporters/otlp/otlptrace/otlptracegrpc/internal/otlpconfig/options.go: Generated file
- exporters/otlp/otlptrace/otlptracehttp/internal/otlpconfig/options.go: Generated file
|
@AbhiPrasad, build is failing. Can you please take a look? |
Add WithMaxResponseBodySize to the OTLP/HTTP trace, metric, and log exporters so users can tune the response body size limit while preserving the 4 MiB default. ```go otlptracehttp.WithMaxResponseBodySize(size int64) otlpmetrichttp.WithMaxResponseBodySize(size int64) otlploghttp.WithMaxResponseBodySize(size int64) ``` Example: ```go exp, err := otlptracehttp.New( otlptracehttp.WithMaxResponseBodySize(8 * 1024 * 1024), ) ``` Passing a size less than or equal to zero disables the response body size limit: ```go exp, err := otlptracehttp.New( otlptracehttp.WithMaxResponseBodySize(0), ) ``` This follows open-telemetry/opentelemetry-proto#802, which made client response limits configurable: open-telemetry/opentelemetry-proto#802 The OTLP/HTTP specification requires clients to limit response body size and says implementations SHOULD allow the limit to be configured: https://github.com/open-telemetry/opentelemetry-proto/blob/41ad255e3efd5450096fc167ef6d87edb4bcb4f3/docs/specification.md#L522-L525
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
8cfe8b7 to
5a23b9e
Compare
9ba2a00 to
23dab0a
Compare
|
@open-telemetry/go-approvers, this needs more reviews and approval(s) |
MrAlias
left a comment
There was a problem hiding this comment.
Before we add this API to three modules, can we establish the concrete use case? The specification's SHOULD is a reason to consider configurability, but the PR does not link a Go issue or describe a legitimate response that exceeds the existing 4 MiB limit. Is there a user who needs to raise or lower this value, or are we treating the SHOULD as sufficient reason to add the permanent API surface?
| // WithMaxResponseBodySize sets the maximum size, in bytes, of the response body | ||
| // (after decompression) that the exporter will read. | ||
| // | ||
| // If size is less than or equal to zero, no response body size limit is applied. |
There was a problem hiding this comment.
The OTLP/HTTP specification says clients MUST limit response-body size to protect against excessive memory use from a misconfigured or malicious server. With size <= 0, each exporter performs an unbounded io.Copy into a bytes.Buffer, so a collector can exhaust the application's memory.
This option can remain configurable, but every configuration needs to retain a finite positive limit. Please treat non-positive values as invalid or keep the 4 MiB default instead of disabling the limit. The same applies to the math.MaxInt64 unlimited path in each exporter.
|
I generally view SHOULD as meaning we should add something unless we have a good reason not to. I could definitely see lowering the value as something I might want to do if large responses were causing problems. |
pellared
left a comment
There was a problem hiding this comment.
Please address #8443 (comment)
|
@AbhiPrasad please address the remaining unresolved thread and current changes-requested review. |
| req.Header.Set("Content-Type", "application/x-protobuf") | ||
|
|
||
| maxResponseBodySize := defaultMaxResponseBodySize | ||
| if cfg.maxResponseBodySize.Set { |
There was a problem hiding this comment.
Could we resolve c.maxResponseBodySize with fallback[int64](defaultMaxResponseBodySize) in newConfig (like maxRequestSize), so we can pass cfg.maxResponseBodySize.Value directly here without manual checking?
Add WithMaxResponseBodySize to the OTLP/HTTP trace, metric, and log exporters so users can tune the response body size limit while preserving the 4 MiB default.
Example:
Passing a size less than or equal to zero disables the response body size limit:
This follows open-telemetry/opentelemetry-proto#802, which made client response limits configurable: open-telemetry/opentelemetry-proto#802
The OTLP/HTTP specification requires clients to limit response body size and says implementations SHOULD allow the limit to be configured: https://github.com/open-telemetry/opentelemetry-proto/blob/41ad255e3efd5450096fc167ef6d87edb4bcb4f3/docs/specification.md#L522-L525