Enforce OTLP request size limits#8446
Conversation
|
280870d to
6720aae
Compare
|
@jkwatson Thanks — I pushed a follow-up that fixes the NullAway compile failure in the OTLP internal exporters and applies the naming nits to use constants and public API parameter names. |
jack-berg
left a comment
There was a problem hiding this comment.
A few small comments, but looks pretty good! Thanks for working on this!
| private final boolean exportAsJson; | ||
| private final long maxRequestBodySize; | ||
|
|
||
| public HttpExporter( |
There was a problem hiding this comment.
This is internal code so we can change the constructor without worrying about breaking any callers. Let's have a single constructor.
| public final class HttpExporterBuilder { | ||
| public static final long DEFAULT_TIMEOUT_SECS = 10; | ||
| public static final long DEFAULT_CONNECT_TIMEOUT_SECS = 10; | ||
| public static final long DEFAULT_MAX_REQUEST_BODY_SIZE = Long.MAX_VALUE; |
There was a problem hiding this comment.
Can we default to 64mb here? Allows the constant to be defined in once place rather than in each of the exporter builders.
| public OtlpGrpcLogRecordExporterBuilder setMaxRequestMessageSize( | ||
| long maxRequestMessageSizeBytes) { | ||
| checkArgument( | ||
| maxRequestMessageSizeBytes >= 0, "maxRequestMessageSizeBytes must be non-negative"); |
There was a problem hiding this comment.
Setting the max body size to 0 is not useful. (Update applies in other places as well)
| maxRequestMessageSizeBytes >= 0, "maxRequestMessageSizeBytes must be non-negative"); | |
| maxRequestMessageSizeBytes > 0, "maxRequestMessageSizeBytes must be positive"); |
| int contentLength = messageWriter.getContentLength(); | ||
| if (contentLength >= 0) { | ||
| return contentLength; | ||
| } |
There was a problem hiding this comment.
Under what conditions do we pass through this?
My read of the code shows that only Marshaler.toJsonMessageWriter produces a MessageWriter.getContentLength which can return a value <= 0.
I don't know that we can cheaply compute the / enforce the body size for OTLP/json. One more reason not to use it. Officially, we don't support it today. See #5833
| @@ -65,15 +86,70 @@ public CompletableResultCode export(Marshaler exportRequest, int numItems) { | |||
| exporterMetrics.startRecordingExport(numItems); | |||
|
|
|||
| CompletableResultCode result = new CompletableResultCode(); | |||
There was a problem hiding this comment.
Since failRequestTooLarge produces its own CompletableResultCode, let's initialize this after line 94.
| ExporterInstrumentation.Recording metricRecording, | ||
| long requestMessageSize) { | ||
| String errorMessage = | ||
| "OTLP gRPC request message size " |
There was a problem hiding this comment.
Let's keep the log message consistent with the shape we use elsewhere in this class when exports fail:
"Failed to export " + type + "s. Request message size " + requestMessageSize + "exceeded limit of " + maxRequestMessageSize + "bytes".
Same advice applies to HttpExporter.
| throw new UnsupportedOperationException("Not implemented"); | ||
| } | ||
|
|
||
| default TelemetryExporterBuilder<T> setMaxRequestMessageSize(long maxRequestMessageSize) { |
There was a problem hiding this comment.
No default needed here. This is an internal interface so better to fail loudly with a compile error if an implementation forgot to implement.
| exporter.export(Collections.singletonList(generateFakeTelemetry())); | ||
|
|
||
| assertThat(result.join(10, TimeUnit.SECONDS).isSuccess()).isFalse(); | ||
| Assertions.assertThat(result.getFailureThrowable()) |
There was a problem hiding this comment.
#nit:
| Assertions.assertThat(result.getFailureThrowable()) | |
| assertThat(result.getFailureThrowable()) |
| Assertions.assertThat(result.getFailureThrowable()) | ||
| .hasMessageContaining("OTLP gRPC request message size") | ||
| .hasMessageContaining("exceeded limit of 1 bytes"); | ||
| } |
There was a problem hiding this comment.
I think there are test methods in these that verify toString. Should update to assert that the new limit field is included in toString.
afad5b0 to
3cf2f56
Compare
Fixes #8444.
Why
opentelemetry-proto#782now recommends that OTLP exporters enforce configurable outbound request/message size limits.What
docs/apidiffs/current_vs_latest/opentelemetry-exporter-otlp.txtfor the new public builder methodsTesting
I was not able to complete local Gradle verification on this Windows machine because Gradle / the Kotlin compiler daemon repeatedly failed to start due to paging-file / metaspace limits before compilation finished. The changes are pushed for CI verification.