[
https://issues.apache.org/jira/browse/HTTPCLIENT-2438?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Arturo Bernal updated HTTPCLIENT-2438:
--------------------------------------
Priority: Minor (was: Major)
> AsyncIoByteCounterExec keeps tags from previous requests
> --------------------------------------------------------
>
> Key: HTTPCLIENT-2438
> URL: https://issues.apache.org/jira/browse/HTTPCLIENT-2438
> Project: HttpComponents HttpClient
> Issue Type: Bug
> Affects Versions: 5.6.3, 5.6.4, 5.6.5
> Reporter: seongsu shin
> Priority: Minor
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h2. Summary
> With the IO metric set on the async client, the request.bytes /
> response.bytes counters can get tags from an earlier request. If one request
> gets a tag that the others don't have, all requests after it are counted with
> that tag too. The classic client is fine.
> h2. Cause
> AsyncIoByteCounterExec creates the two Counter.Builder instances once in the
> constructor and keeps them in fields. record() then does this for every
> request:
> {code:java}
> reqBuilder.tags(tags).tags(mc.commonTags).register(meterRegistry).increment(reqBytes);
> {code}
> Counter.Builder.tags() adds to the tags the builder already has, it doesn't
> replace them. method and status are on every request so they just get
> overwritten, but any other tag stays on the builder and is used for every
> request after that.
> IoByteCounterExec creates a new builder on each call, and there is a comment
> there saying it's done to avoid tag accumulation, so I guess the async one
> was just missed.
> h2. Why it matters
> If you use a tagCustomizer that adds a tag only for some requests (tenant,
> endpoint name, etc.), the byte counters for that tag end up including traffic
> that has nothing to do with it. Nothing fails or gets logged, so the numbers
> just look normal but are wrong.
> Also the same builder is shared across all requests of the client and
> Counter.Builder isn't thread safe, so concurrent requests could mix their
> tags as well. I didn't try to reproduce that part.
> h2. How to reproduce
> Async client, IO metric set, and a tagCustomizer that adds a tag only for /a:
> {code:java}
> ObservingOptions opts = ObservingOptions.builder()
> .metrics(EnumSet.of(ObservingOptions.MetricSet.IO))
> .tagCustomizer((tags, method, status, protocol, target, uri) -> {
> List<Tag> out = new ArrayList<>(tags);
> if (uri.startsWith("/a")) {
> out.add(Tag.of("only_for_a", "yes"));
> }
> return out;
> })
> .build();
> HttpClientObservationSupport.enable(builder, null, meterRegistry, opts,
> MetricConfig.DEFAULT);
> {code}
> Send GET /a (200), then GET /b (404), and look at the response.bytes
> counters. Did the same with the classic client to compare.
> h2. Observed
> {noformat}
> async:
> total=4 [method=GET, only_for_a=yes, status=200]
> total=2 [method=GET, only_for_a=yes, status=404]
> classic:
> total=4 [method=GET, only_for_a=yes, status=200]
> total=2 [method=GET, status=404]
> {noformat}
> /b has only_for_a=yes on the async client.
> h2. Expected
> /b should only have its own tags, same as classic.
> h2. Notes
> Creating the builders inside record() like IoByteCounterExec does should fix
> it. I can open a PR with a test if that sounds ok.
> PR #920 also changes this class (uri tag) but not this part.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
