Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumentations for Apache HttpComponents do not meter errors and leak memory #3800

Closed
cachescrubber opened this issue May 2, 2023 · 3 comments
Assignees
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Milestone

Comments

@cachescrubber
Copy link
Contributor

Describe the bug
An HttpAsyncClient instrumented with MicrometerHttpClientInterceptor does not meter IO errors.
For example, requests resulting in a ConnectTimeoutException, SocketTimeoutException are not metered.

MicrometerHttpRequestExecutor however does meter IO errors by setting the status key to IO_ERROR.

Environment

  • Micrometer version [1.11.0-RC1]
  • Micrometer registry [prometheus]
  • Java version: [17]

To Reproduce
How to reproduce the bug:
Make an instrumented client to run into some sort of IO Exception, ConnectTimeoutException, SocketTimeoutException, etc.

Expected behavior

  • The request is counted, and elapsed time is measured.
  • The Tag status has the value IO_ERROR
  • The Tag outcome is UNKNOWN.

Additional context
The Apache Http Components instrumentation does not support the tag exception, which is quite commonly used by other http client instrumentations.

@shakuzen shakuzen added enhancement A general enhancement module: micrometer-core An issue that is related to our core module instrumentation An issue that is related to instrumenting a component labels May 8, 2023
@shakuzen shakuzen added this to the 1.12 backlog milestone May 8, 2023
@cachescrubber
Copy link
Contributor Author

cachescrubber commented May 10, 2023

I dig a bit deeper into this. As it stands right now, The httpcomponents binder is not able to meter connection related IO errors like connection-refused or connection-timeout, TLS handshake related errors, etc. This affects both the classic and the async client. I committed test cases to #3801 to highlight the issue. Bummer.

Implemented in #3801 is a measurement of IO errors during an active http request, for example socket read timeouts. This brings the async implementation on par with classic.

@cachescrubber
Copy link
Contributor Author

As it stands right now, the httpcomponents binder does not only miss connection related errors - the time necessary to establish a connection is not metered at all. #3801 has changed the instrumentation to use ExecChainHandler instead of Request/Response interceptor to address this issue.

@cachescrubber
Copy link
Contributor Author

Backport to 4.x would require to Subclass HttpClientBuilder and override its #decorateMainExec or #decorateProtocolExec methods.

@bclozel bclozel self-assigned this Jun 23, 2023
@bclozel bclozel modified the milestones: 1.12 backlog, 1.12.0-M1 Jun 26, 2023
@bclozel bclozel changed the title MicrometerHttpClientInterceptor for Apache HttpAsyncClient does not meter io errors Instrumentations for Apache HttpComponents do not meter errors and leak memory Jun 26, 2023
bclozel pushed a commit that referenced this issue Jun 26, 2023
This commit replaces the request and response Interceptors for
the "classic" and "async" clients by `AsyncExecChainHandler`
and `ExecChainHandler` implementations.

See gh-3800
bclozel added a commit that referenced this issue Jun 26, 2023
This commit reinstates the former HttpComponents instrumentation for 4.x
and 5.x, in a deprecated fashion for 5.x since it will be replaced by
the `ObservationExecChainHandler` in this dedicated issue.

See gh-3800
bclozel added a commit that referenced this issue Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement instrumentation An issue that is related to instrumenting a component module: micrometer-core An issue that is related to our core module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants