Skip to content

feat: Add peer address to downstream handshake error logs#536

Closed
jsulmont wants to merge 9 commits intocloudflare:mainfrom
jsulmont:fix/add-peer-addr-to-handshake-errors
Closed

feat: Add peer address to downstream handshake error logs#536
jsulmont wants to merge 9 commits intocloudflare:mainfrom
jsulmont:fix/add-peer-addr-to-handshake-errors

Conversation

@jsulmont
Copy link
Copy Markdown

Add peer address to handshake error logs

  • Adds peer address information to downstream handshake error logs
  • Improves debugging capabilities by showing which client caused handshake failures
  • Maintains existing error logging flow with minimal code changes

Fixes #535

Comment thread pingora-core/src/services/listening.rs
@jsulmont
Copy link
Copy Markdown
Author

@drcaramelsyrup I am not sure why the build 1.72.0 failed -- the previous push passed both 1.72 and 1.82 (failed bnightly)

@matthewbjones
Copy link
Copy Markdown
Contributor

@jsulmont the build failure in 1.72.0 should be resolved now that #538 was just merged a few minutes ago

@drcaramelsyrup
Copy link
Copy Markdown
Collaborator

drcaramelsyrup commented Feb 28, 2025

@drcaramelsyrup I am not sure why the build 1.72.0 failed -- the previous push passed both 1.72 and 1.82 (failed bnightly)

@jsulmont Sorry, I realize I led you down the wrong path with my suggestion. Your original change actually seems fine now, because I was mistaken - today we already set the peer_addr in the digest upon accepting, so deferring that syscall doesn't really do anything.

As it stands it's not possible to do what I had originally suggested because the handshake function moves the io stream.

Comment thread pingora-core/src/listeners/mod.rs Outdated
@jsulmont
Copy link
Copy Markdown
Author

@matthewbjones there's now a test failing for 1.72 in pingora-memory-cache :/

@drcaramelsyrup
Copy link
Copy Markdown
Collaborator

Looks like it was a flaky test. We'll get this change reviewed internally.

Copy link
Copy Markdown
Collaborator

@drcaramelsyrup drcaramelsyrup left a comment

Choose a reason for hiding this comment

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

This now essentially lgtm will some minor nits I'd like to see, I can also make these changes in a separate commit internally if you'd like.

Comment thread pingora-core/src/listeners/mod.rs Outdated
Comment thread pingora-core/src/services/listening.rs Outdated
@jsulmont
Copy link
Copy Markdown
Author

jsulmont commented Mar 1, 2025

This now essentially lgtm will some minor nits I'd like to see, I can also make these changes in a separate commit internally if you'd like.

All good, I've pushed a commit towards addressing your two comments above -- thanks @drcaramelsyrup

@jsulmont
Copy link
Copy Markdown
Author

jsulmont commented Mar 5, 2025

@drcaramelsyrup still the tests::test_eviction failing

@jsulmont jsulmont requested a review from drcaramelsyrup March 7, 2025 17:52
@drcaramelsyrup drcaramelsyrup added the Accepted This change is accepted by us and merged to our internal repo label Mar 8, 2025
@drcaramelsyrup
Copy link
Copy Markdown
Collaborator

Thanks, we've merged internally and the change should be present in a future sync.

@jsulmont jsulmont requested a review from drcaramelsyrup March 8, 2025 07:13
@jsulmont
Copy link
Copy Markdown
Author

Thanks, we've merged internally and the change should be present in a future sync.

Thank you 🙏🏻

drcaramelsyrup pushed a commit that referenced this pull request Apr 18, 2025
---
refactor: replace match with let binding for cleaner code
---
fix: optimize syscall usage in handshake error logging
---
Revert "fix: optimize syscall usage in handshake error logging"

This reverts commit 0724afc.
---
Refactor peer_addr() using method chaining with and_then/map combinators
---
Merge remote-tracking branch 'upstream/main' into fix/add-peer-addr-to-handshake-errors
---
fix: Use proper SocketAddr type and move peer address retrieval inside async closure

Includes-commit: 0724afc
Includes-commit: 20b2e8f
Includes-commit: 224ea8a
Includes-commit: 71e43d4
Includes-commit: c2333b3
Includes-commit: cfa7d33
Includes-commit: eecf06d
Replicated-from: #536
drcaramelsyrup pushed a commit that referenced this pull request Apr 18, 2025
---
refactor: replace match with let binding for cleaner code
---
fix: optimize syscall usage in handshake error logging
---
Revert "fix: optimize syscall usage in handshake error logging"

This reverts commit 0724afc.
---
Refactor peer_addr() using method chaining with and_then/map combinators
---
Merge remote-tracking branch 'upstream/main' into fix/add-peer-addr-to-handshake-errors
---
fix: Use proper SocketAddr type and move peer address retrieval inside async closure

Includes-commit: 0724afc
Includes-commit: 20b2e8f
Includes-commit: 224ea8a
Includes-commit: 71e43d4
Includes-commit: c2333b3
Includes-commit: cfa7d33
Includes-commit: eecf06d
Replicated-from: #536
@andrewhavck
Copy link
Copy Markdown
Collaborator

This landed in our sync yesterday, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Accepted This change is accepted by us and merged to our internal repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add peer address to handshake error logs

4 participants