Skip to content

Add logging to the new HTTPConnectionPool #428

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

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

fabianfett
Copy link
Member

Motivation

To debug the new HTTPConnectionPool and Connections, it would be great to get a rather complete event story.

Changes

  • Added a number of trace logs
  • Added a nicer debug output for Connection.Key

Result

A better debuggable AHC client.

@fabianfett fabianfett added the 🔨 semver/patch No public API change. label Sep 15, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 15, 2021
@fabianfett fabianfett requested review from weissi and Lukasa September 15, 2021 10:01
Comment on lines -134 to -139
didSet {
self.logger.trace("Connection Pool State changed", metadata: [
"key": "\(self.key)",
"state": "\(self._state)",
])
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed, since it produces way too many logs. We should solely focus on recording the events so that we can replay the finite state machine if needed.

Comment on lines -147 to -149
if let idleReadTimeout = self.request?.idleReadTimeout {
self.idleReadTimeoutStateMachine = .init(timeAmount: idleReadTimeout)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be removed, since it is already included in the request setter (l. 143). We have tests that ensure that idle read timeouts work as expected. Those continue to succeed.

@fabianfett fabianfett merged commit 7c9662d into swift-server:main Sep 20, 2021
@fabianfett fabianfett deleted the ff-logging branch September 20, 2021 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants