Conversation
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8224 +/- ##
============================================
- Coverage 90.31% 90.29% -0.02%
- Complexity 7652 7662 +10
============================================
Files 843 845 +2
Lines 23087 23140 +53
Branches 2312 2323 +11
============================================
+ Hits 20851 20895 +44
- Misses 1517 1521 +4
- Partials 719 724 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
exporters/common/src/main/java/io/opentelemetry/exporter/internal/grpc/GrpcExporterBuilder.java
Show resolved
Hide resolved
...ender/jdk/src/test/java/io/opentelemetry/exporter/sender/jdk/internal/JdkHttpSenderTest.java
Show resolved
Hide resolved
| @Nullable RetryPolicy retryPolicy, | ||
| @Nullable ExecutorService executorService) { | ||
| @Nullable ExecutorService executorService, | ||
| long maxResponseBodySize) { |
There was a problem hiding this comment.
JaegerRemoteSamplerBuilder.setMaxSamplingStrategyResponseBodySize validates bytes > 0. But the sender constructors (JdkHttpSender, OkHttpHttpSender, OkHttpGrpcSender) accept any long without
validation. A caller can bypass the guard by constructing a sender directly with 0 or -1. Consider adding the validation at the sender level too, or document the expected invariant.
There was a problem hiding this comment.
All the sender constructors are internal, and have a bunch of other unvalidated parameters which could be equally corrupted if a user goes around the guards. There's a conversation here discussing where to add additional null checks beyond the guarantees from nullaway. I think we should adopt a policy of adding additional null checks at the API boundaries, but trusting nullaway once we're within the walled garden of internal code. The same would apply to parameter validation, I think.
.../okhttp/src/main/java/io/opentelemetry/exporter/sender/okhttp/internal/OkHttpHttpSender.java
Outdated
Show resolved
Hide resolved
...rs/sender/jdk/src/main/java/io/opentelemetry/exporter/sender/jdk/internal/JdkHttpSender.java
Show resolved
Hide resolved
| * Sets the maximum number of bytes to read from a sampling strategy response body. If unset, | ||
| * defaults to 4 MiB. Must be positive. | ||
| */ | ||
| public JaegerRemoteSamplerBuilder setMaxSamplingStrategyResponseBodySize(long bytes) { |
There was a problem hiding this comment.
GrpcExporterBuilder and HttpExporterBuilder hardcode 4MB with no setter. Only JaegerRemoteSamplerBuilder exposes a public setter. If a user wants to tighten the OTLP limit (say, to 64KB since responses are tiny), they can't. Worth a deliberate decision: intentionally omit setters for OTLP (since responses are well-known small), or add them for consistency?
There was a problem hiding this comment.
I omitted them in OTLP for now because we don't do anything with them and a 4mb limit is an improvement on the current no limit.
In contrast, for JaegerRemoteSampler, the default body size reduction is a change in behavior which could be meaningful so adding the setter seems essential.
…y-java into response-body-bounds
Add new max response body size parameter to sender config, defaulting to 4mb.
Per the spec, if a response exceeding the limit is received:
For OTLP exporters, the body size is not configurable, since we don't do anything with the response today. If adding this 4mb limit causes new exceptions for users, we can add a config option to allow it to be increased.
For JaegerRemoteSampler, a new programmatic config option is added because the responses are critical to the function of the sampler.
cc @brunobat you'll want to update the vertex sender to incorporate this new option.