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

Cleanup: Logging and some more #447

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MahdiBM
Copy link
Contributor

@MahdiBM MahdiBM commented Jan 1, 2024

No description provided.

@@ -563,8 +563,8 @@ final class PostgresChannelHandler: ChannelDuplexHandler {
_ cleanup: ConnectionStateMachine.ConnectionAction.CleanUpContext,
context: ChannelHandlerContext
) {
self.logger.debug("Cleaning up and closing connection.", metadata: [.error: "\(cleanup.error)"])

self.logger.debug("Cleaning up and closing connection.", metadata: [.error: "\(String(reflecting: cleanup.error))"])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example of what it solves:

Screenshot 2024-01-01 at 9 20 09 PM

@@ -280,7 +280,7 @@ final class PSQLRowStream: @unchecked Sendable {
precondition(!newRows.isEmpty, "Expected to get rows!")
self.eventLoop.preconditionInEventLoop()
self.logger.trace("Row stream received rows", metadata: [
"row_count": "\(newRows.count)"
.rowCount: "\(newRows.count)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

inconsistency with all other log statements.

@@ -803,7 +803,7 @@ final class ConnectionPoolTests: XCTestCase {

pool.connectionReceivedNewMaxStreamSetting(connection, newMaxStreamSetting: 21)

for (index, request) in requests.enumerated() {
for (_, request) in requests.enumerated() {
Copy link
Contributor Author

@MahdiBM MahdiBM Jan 1, 2024

Choose a reason for hiding this comment

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

warning for unused variable.

@@ -22,7 +22,6 @@ extension PostgresJSONDecoder {
}
}

//@available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *)
Copy link
Contributor Author

@MahdiBM MahdiBM Jan 1, 2024

Choose a reason for hiding this comment

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

There are some Sendable warning for these 2 JSON-Coder conformances. I didn't try to solve them because someone (Fabian probably?) tried to solve them but apparently stopped at some point (judging by the comment).

@codecov-commenter
Copy link

codecov-commenter commented Jan 1, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (e9b90b2) 62.18% compared to head (cdddfe9) 62.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #447      +/-   ##
==========================================
+ Coverage   62.18%   62.20%   +0.02%     
==========================================
  Files         124      124              
  Lines        9936     9936              
==========================================
+ Hits         6179     6181       +2     
+ Misses       3757     3755       -2     
Files Coverage Δ
...rces/PostgresNIO/New/Extensions/Logging+PSQL.swift 23.33% <ø> (ø)
...urces/PostgresNIO/New/PostgresChannelHandler.swift 84.77% <100.00%> (ø)
...es/PostgresNIO/Utilities/PostgresJSONDecoder.swift 50.00% <ø> (ø)
Sources/PostgresNIO/New/PSQLRowStream.swift 86.33% <0.00%> (ø)

... and 1 file with indirect coverage changes

@MahdiBM MahdiBM marked this pull request as ready for review January 1, 2024 18:02
@MahdiBM MahdiBM requested review from gwynne and fabianfett and removed request for gwynne January 2, 2024 14:52
@fabianfett
Copy link
Collaborator

Related #411.

@fabianfett
Copy link
Collaborator

Logging as well as Errors currently suck in PostgresNIO. I'm allowed to say that, as I was the one who implemented those. We should come up with a better logging and error plan first. Sadly I'm currently extremely low on time. @MahdiBM would you be interested in moving this forward?

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jan 30, 2024

@fabianfett sure, but I don't have a plan of myself. How should we proceed?

@MahdiBM
Copy link
Contributor Author

MahdiBM commented Jan 30, 2024

<code-ql action is broken everywhere for now>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants