Repository navigation
Conversation
arturobernalg
left a comment
There was a problem hiding this comment.
I do not think getContentLength() is sufficient for these metrics. It reports the declared entity length, not necessarily the number of bytes transferred.
The tests even record a positive request body size when the connection is refused and no body is sent.
If we use the OpenTelemetry metric names, the recorded values should follow their semantics. Otherwise, these should be exposed as declared entity lengths under different names.
65ebeda to
536bab3
Compare
|
Thanks, you are right.
Why the declared, uncompressed length is still useful: servers often limit If you wish, I can add transferred body sizes under the OpenTelemetry names in a |
arturobernalg
left a comment
There was a problem hiding this comment.
Thanks for the update. The metric semantics are now clear and the interceptors themselves look fine.
There are still a couple of limitations worth addressing.
The caching builder overloads currently do not install the interceptors because CACHING is added after the custom interceptor entries are processed. This is a pre-existing issue, which I will address separately. We should not claim caching support until it is covered by integration tests.
Also, HttpCore reports an unknown response entity length over HTTP/2 even when Content-Length is present. This should be documented.
Finally, getContentLength() does not necessarily represent the uncompressed entity length. I would avoid making that claim unconditionally.
Add `ObservingOptions.MetricSet.CONTENT_LENGTH`, which installs the new `ContentLengthExec` (classic) and `AsyncContentLengthExec` (async) interceptors. They record `<prefix>.request.content.length` and `<prefix>.response.content.length` as `DistributionSummary` meters in bytes. The values are the declared entity lengths as the application sees them: `HttpEntity#getContentLength()` (classic), `AsyncEntityProducer#getContentLength()` and `EntityDetails#getContentLength()` (async). The interceptors run before the client encodes the request and after it decodes the response, so when the client handles content coding the values are the uncompressed lengths. They are the encoded lengths if the application sends an already encoded entity or if content compression is disabled. They are not the number of bytes transferred, so the OpenTelemetry names `http.client.request.body.size` / `http.client.response.body.size` (defined as transferred, possibly compressed bytes) are not used. The uncompressed length matters because servers often limit the decoded size of a request, not its size on the wire. For example, OpenSearch rejects requests with `413 Request Entity Too Large` when the body exceeds `http.max_content_length` (default `100mb`). Netty's `HttpObjectAggregator` enforces this limit after `HttpContentDecompressor`, so a small gzip body that inflates past the limit is rejected. This metric shows how close clients get to such a limit, for example with large `_bulk` requests. See https://github.com/opensearch-project/OpenSearch/blob/56bfaa9cc8c5ba4ee49a92e3476dd9898b76e6e3/modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpServerTransport.java#L500-L506 Nothing is recorded if no response was received, for entities of unknown length (chunked, transparently decompressed) or for messages without an entity. Over HTTP/2, HttpCore reports the response entity length as unknown, so no response length is recorded there. Entities are never wrapped or buffered. Tags match the request timer (`method`, `status`, plus `protocol` and `target` at `TagLevel.EXTENDED`, common tags, tag customizer). The existing `IO` byte counters and the default metric set are unchanged; the new meters are opt-in. See micrometer-metrics/micrometer#880 and micrometer-metrics/micrometer#7158. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Thanks! I have addressed the points:
|
536bab3 to
74d9bc1
Compare
|
@rursprung Could you demonstrate how this metric captures the uncompressed request size when using |
|
oh, i didn't even notice that because so for our setup (no gzip enabled on our side) the functionality from this PR is still useful and i presume there are many setups out there where this would also be the case. does this PR still make sense to you? (i'd of course change the javadoc & commit-msg to reflect the new facts) |
|
I am not convinced this feature justifies the additional public API and maintenance overhead. It only records known entity lengths, not actual content sizes or bytes transferred. It does not cover compressed or streaming requests, and the original OpenSearch use case is only partially addressed. Applications can already obtain declared lengths through I would prefer not to merge this in its current form. I think we should reconsider whether HttpClient should expose declared entity lengths or actual content size metrics. |
|
Hi @rursprung, thanks for working on this! 🙂 I was curious how the new meters behave with gzip and HTTP/2, so I tried the PR (74d9bc1) against a small local server. I'm sharing the numbers in case they help. Setup:
So I couldn't find a case where the uncompressed size shows up 😅 When something is recorded, it's the same as the For the OpenSearch case I think this means the decoded If you want to try it yourself, drop the test below into All 4 tests pass on the PR branch. The assertions just describe what happens today. Hope this is useful! 🙏 ContentLengthSemanticsTest.javapackage org.apache.hc.client5.http.observation.interceptors;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNull;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.util.Arrays;
import java.util.EnumSet;
import java.util.concurrent.TimeUnit;
import java.util.zip.GZIPOutputStream;
import io.micrometer.core.instrument.DistributionSummary;
import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import org.apache.hc.client5.http.async.methods.SimpleHttpResponse;
import org.apache.hc.client5.http.async.methods.SimpleRequestBuilder;
import org.apache.hc.client5.http.classic.methods.HttpPost;
import org.apache.hc.client5.http.entity.EntityBuilder;
import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient;
import org.apache.hc.client5.http.impl.async.HttpAsyncClientBuilder;
import org.apache.hc.client5.http.impl.async.HttpAsyncClients;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
import org.apache.hc.client5.http.impl.classic.HttpClients;
import org.apache.hc.client5.http.observation.HttpClientObservationSupport;
import org.apache.hc.client5.http.observation.MetricConfig;
import org.apache.hc.client5.http.observation.ObservingOptions;
import org.apache.hc.core5.http.ContentType;
import org.apache.hc.core5.http.EntityDetails;
import org.apache.hc.core5.http.HttpException;
import org.apache.hc.core5.http.HttpRequest;
import org.apache.hc.core5.http.Message;
import org.apache.hc.core5.http.URIScheme;
import org.apache.hc.core5.http.impl.bootstrap.HttpAsyncServer;
import org.apache.hc.core5.http.impl.bootstrap.HttpServer;
import org.apache.hc.core5.http.impl.bootstrap.ServerBootstrap;
import org.apache.hc.core5.http.io.entity.ByteArrayEntity;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.apache.hc.core5.http.message.BasicHttpResponse;
import org.apache.hc.core5.http.nio.AsyncRequestConsumer;
import org.apache.hc.core5.http.nio.AsyncServerRequestHandler;
import org.apache.hc.core5.http.nio.entity.BasicAsyncEntityConsumer;
import org.apache.hc.core5.http.nio.entity.BasicAsyncEntityProducer;
import org.apache.hc.core5.http.nio.support.BasicRequestConsumer;
import org.apache.hc.core5.http.nio.support.BasicResponseProducer;
import org.apache.hc.core5.http.protocol.HttpContext;
import org.apache.hc.core5.http2.HttpVersionPolicy;
import org.apache.hc.core5.http2.impl.nio.bootstrap.H2ServerBootstrap;
import org.apache.hc.core5.io.CloseMode;
import org.apache.hc.core5.reactor.ListenerEndpoint;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
/**
* What CONTENT_LENGTH records when compression or HTTP/2 is involved.
* Body: 100_000 bytes of 'a' (gzips to 133 bytes). Assertions describe the observed behaviour.
*/
class ContentLengthSemanticsTest {
private static final String REQUEST = "t.request.content.length";
private static final String RESPONSE = "t.response.content.length";
private static final byte[] BODY = new byte[100_000];
static {
Arrays.fill(BODY, (byte) 'a');
}
private byte[] gzippedBody;
private HttpServer server;
private String base;
private volatile int serverReceivedBytes;
private static ObservingOptions options() {
return ObservingOptions.builder().metrics(EnumSet.of(ObservingOptions.MetricSet.CONTENT_LENGTH)).build();
}
private static MetricConfig config() {
return MetricConfig.builder().prefix("t").build();
}
private static Double total(final MeterRegistry meters, final String name) {
final DistributionSummary s = meters.find(name).summary();
return s != null ? s.totalAmount() : null;
}
@BeforeEach
void setUp() throws IOException {
final ByteArrayOutputStream out = new ByteArrayOutputStream();
try (GZIPOutputStream gz = new GZIPOutputStream(out)) {
gz.write(BODY);
}
gzippedBody = out.toByteArray();
server = ServerBootstrap.bootstrap()
.setLocalAddress(InetAddress.getLoopbackAddress())
.setListenerPort(0)
.register("localhost", "/echo", (request, response, context) -> {
serverReceivedBytes = EntityUtils.toByteArray(request.getEntity()).length;
response.setCode(200);
})
.register("localhost", "/gzip", (request, response, context) -> {
response.setCode(200);
response.setEntity(new ByteArrayEntity(gzippedBody, ContentType.TEXT_PLAIN, "gzip"));
})
.create();
server.start();
base = "http://localhost:" + server.getLocalPort();
}
@AfterEach
void tearDown() {
server.close(CloseMode.IMMEDIATE);
}
private MeterRegistry classic(final HttpPost post) throws IOException {
final MeterRegistry meters = new SimpleMeterRegistry();
final HttpClientBuilder builder = HttpClients.custom();
HttpClientObservationSupport.enable(builder, null, meters, options(), config());
try (CloseableHttpClient client = builder.build()) {
client.execute(post, r -> {
EntityUtils.consume(r.getEntity());
return null;
});
}
return meters;
}
@Test
void requestCompressedWithEntityBuilderIsNotRecorded() throws Exception {
final HttpPost post = new HttpPost(base + "/echo");
post.setEntity(EntityBuilder.create().setBinary(BODY)
.setContentType(ContentType.APPLICATION_JSON).gzipCompressed().build());
final MeterRegistry meters = classic(post);
assertEquals(gzippedBody.length, serverReceivedBytes, "server received the gzipped body");
assertNull(total(meters, REQUEST), "neither the decoded nor the wire size is recorded");
}
@Test
void preEncodedRequestRecordsCompressedLength() throws Exception {
final HttpPost post = new HttpPost(base + "/echo");
post.setEntity(new ByteArrayEntity(gzippedBody, ContentType.APPLICATION_JSON, "gzip"));
final MeterRegistry meters = classic(post);
assertEquals(gzippedBody.length, serverReceivedBytes);
assertEquals((double) gzippedBody.length, total(meters, REQUEST), "compressed length, not 100000");
}
@Test
void transparentlyDecompressedResponseIsNotRecorded() throws Exception {
final MeterRegistry meters = classic(new HttpPost(base + "/gzip"));
assertNull(total(meters, RESPONSE), "Content-Length: 133 on the wire, 100000 read by the app, nothing recorded");
}
@Test
void http2ResponseWithContentLengthIsNotRecorded() throws Exception {
final HttpAsyncServer h2 = H2ServerBootstrap.bootstrap()
.setVersionPolicy(HttpVersionPolicy.FORCE_HTTP_2)
.register("*", new AsyncServerRequestHandler<Message<HttpRequest, byte[]>>() {
@Override
public AsyncRequestConsumer<Message<HttpRequest, byte[]>> prepare(
final HttpRequest request, final EntityDetails details, final HttpContext context) {
return new BasicRequestConsumer<>(details != null ? new BasicAsyncEntityConsumer() : null);
}
@Override
public void handle(final Message<HttpRequest, byte[]> message, final ResponseTrigger trigger,
final HttpContext context) throws HttpException, IOException {
final BasicHttpResponse head = new BasicHttpResponse(200);
head.addHeader("Content-Length", Integer.toString(BODY.length));
trigger.submitResponse(new BasicResponseProducer(head,
new BasicAsyncEntityProducer(BODY, ContentType.APPLICATION_OCTET_STREAM)), context);
}
})
.create();
h2.start();
final ListenerEndpoint endpoint = h2.listen(
new InetSocketAddress(InetAddress.getLoopbackAddress(), 0), URIScheme.HTTP).get();
final int port = ((InetSocketAddress) endpoint.getAddress()).getPort();
final MeterRegistry meters = new SimpleMeterRegistry();
final HttpAsyncClientBuilder builder = HttpAsyncClients.custom().setVersionPolicy(HttpVersionPolicy.FORCE_HTTP_2);
HttpClientObservationSupport.enable(builder, null, meters, options(), config());
try (CloseableHttpAsyncClient client = builder.build()) {
client.start();
final SimpleHttpResponse response = client.execute(
SimpleRequestBuilder.get("http://localhost:" + port + "/").build(), null).get(20, TimeUnit.SECONDS);
assertEquals("100000", response.getFirstHeader("Content-Length").getValue());
assertNull(total(meters, RESPONSE), "HTTP/2 response with Content-Length: 100000, nothing recorded");
} finally {
h2.close(CloseMode.IMMEDIATE);
}
}
} |
(full disclosure: AI-generated, reviewed by me)
What
Adds opt-in metrics for the declared request and response entity lengths to
httpclient5-observation, for classic and async clients:<prefix>.request.content.lengthDistributionSummary<prefix>.response.content.lengthDistributionSummaryEnable with the new
ObservingOptions.MetricSet.CONTENT_LENGTH:The classic and async
enable(...)variants install the newContentLengthExec/AsyncContentLengthExecinterceptors. The interceptorsare public, so they can also be added by hand. The caching
enable(...)variants contain the same wiring as for
BASICandIO, but the cachingbuilders currently add
CACHINGafter custom interceptors are processed, sonone of these interceptors are installed there yet. That is a pre-existing
issue that will be fixed separately; this PR does not claim caching support.
Why the declared length
Many servers limit the size of a request after decoding, not its size on
the wire. Our use case is OpenSearch, which is used through HttpClient 5 by
opensearch-java:413 Request Entity Too Largeif itsbody exceeds
http.max_content_length(default100mb)(setting).
decoder -> HttpContentDecompressor -> HttpObjectAggregator(maxContentLength).The aggregator enforces the limit after decompression, so it counts the
inflated bytes. A small gzip body that inflates past the limit is rejected
(pipeline).
So to see how close large
_bulkrequests get to the limit, users need theuncompressed size. When the client handles content coding, the declared
entity length that the client sees before compression is exactly that. The
wire size would not show this.
Background:
metrics. The Micrometer team said
(comment)
that this must be done per HTTP library, because
Content-Lengthismissing for chunked bodies and compression makes body size differ from
bytes on the network.
(since 1.17.0) in favor of
httpclient5-observation, so new HttpClient 5instrumentation belongs here.
Design
IOcounters(
<prefix>.request.bytes/<prefix>.response.bytes) are unchanged.ObservingOptions.DEFAULT(BASIC only) is unchanged. Note:ObservingOptions.allMetricSets()now includesCONTENT_LENGTH. japicmpreports the enum change as compatible.
<prefix>.request.content.length/<prefix>.response.content.length. These are deliberately not theOpenTelemetry names
http.client.request.body.size/http.client.response.body.size: OTel defines those as bytes transferred,compressed if a content coding is used. There is no current OTel name for
the uncompressed size (the old
http.*_content_length_uncompressedattributes are deprecated).
DistributionSummary, base unitbytes.MetricConfig.percentilesare published if configured.
method,status, and withTagLevel.EXTENDEDalsoprotocolandtarget.MetricConfig.commonTagsandObservingOptions.tagCustomizerapply.HttpEntity#getContentLength()of the request andresponse entities. Async:
AsyncEntityProducer#getContentLength()andEntityDetails#getContentLength(). The interceptors sit at the start ofthe chain, so they see the request before the client compresses it and the
response after the client decompresses it. The value is therefore
uncompressed when the client handles content coding. It is the encoded
length if the application sends an already encoded entity (for example
with its own
Content-Encoding: gzip) or if content compression isdisabled.
wrapped or buffered.
spanSamplingis honored.What is and is not recorded
that (for example connection refused), nothing is recorded.
Transfer-Encoding: chunked,getContentLength() == -1), including responses that the clientdecompresses transparently (the decompressing wrappers report
-1), andmessages without an entity.
if
Content-Lengthis present, so no response length is recorded forHTTP/2 exchanges.
transferred: it excludes headers and framing, and is
recorded even if the body was not fully sent or received (for example a
server that rejects a request before reading its body).
Possible follow-ups (separate PRs)
http.client.request.body.size/http.client.response.body.size:interceptor just before
MAIN_TRANSPORT(after compression) plus countingof the bytes actually written and read. This also covers chunked bodies.
wrapping entities.
Tests
ContentLengthExecTest(classic) andAsyncContentLengthExecTest(async): known length, chunked/unknown length, no entity (
GET/204),GETwith response entity, error response (500), no response(connection refused, nothing recorded), extended and common tags,
spanSamplingskip, default options not installing the meters../mvnw -pl httpclient5-observation verify javadoc:javadocpasses: tests,checkstyle, japicmp (against 5.6), Javadoc and apache-rat.
Docs
observation.md) lives inapache/httpcomponents-websiteand has no 5.7.x version yet. I'll raise a follow-up PR there once this PR here is approved/merged.