Skip to content

Conversation

eserilev
Copy link
Member

@eserilev eserilev commented Jul 25, 2023

Issue Addressed

#4538

Proposed Changes

add newtype wrapper around DialError that extracts error messages and logs them in a more readable format

Additional Info

I was able to test Transport Dial Errors in the situation where a libp2p instance attempts to ping a nonexistent peer. That error message should look something like

A transport level error has ocurred: Connection refused (os error 61)

AgeManning mentioned we should try fetching only the most inner error (in situations where theres a nested error). I took a stab at implementing that

For non transport DialErrors, I wrote out the error messages explicitly (as per the docs). Could potentially clean things up here if thats not necessary

@paulhauner paulhauner added the work-in-progress PR is a work-in-progress label Jul 26, 2023
@eserilev eserilev marked this pull request as ready for review July 26, 2023 21:38
@eserilev eserilev changed the title [WIP] Improve transport connection errors Improve transport connection errors Jul 26, 2023
@eserilev eserilev marked this pull request as draft July 26, 2023 21:42
@eserilev eserilev marked this pull request as ready for review July 27, 2023 09:28
@AgeManning
Copy link
Member

This looks great. Thanks for the great work!

@AgeManning AgeManning added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Aug 9, 2023
Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@AgeManning AgeManning added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 9, 2023
@AgeManning
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Aug 9, 2023
## Issue Addressed

#4538 

## Proposed Changes

add newtype wrapper around DialError that extracts error messages and logs them in a more readable format

## Additional Info

I was able to test Transport Dial Errors in the situation where a libp2p instance attempts to ping a nonexistent peer. That error message should look something like

`A transport level error has ocurred: Connection refused (os error 61)`

AgeManning mentioned we should try fetching only the most inner error (in situations where theres a nested error). I took a stab at implementing that

For non transport DialErrors, I wrote out the error messages explicitly (as per the docs). Could potentially clean things up here if thats not necessary


Co-authored-by: Age Manning <[email protected]>
@bors
Copy link

bors bot commented Aug 9, 2023

Build failed:

@AgeManning
Copy link
Member

bors retry

bors bot pushed a commit that referenced this pull request Aug 10, 2023
## Issue Addressed

#4538 

## Proposed Changes

add newtype wrapper around DialError that extracts error messages and logs them in a more readable format

## Additional Info

I was able to test Transport Dial Errors in the situation where a libp2p instance attempts to ping a nonexistent peer. That error message should look something like

`A transport level error has ocurred: Connection refused (os error 61)`

AgeManning mentioned we should try fetching only the most inner error (in situations where theres a nested error). I took a stab at implementing that

For non transport DialErrors, I wrote out the error messages explicitly (as per the docs). Could potentially clean things up here if thats not necessary


Co-authored-by: Age Manning <[email protected]>
@bors
Copy link

bors bot commented Aug 10, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Improve transport connection errors [Merged by Bors] - Improve transport connection errors Aug 10, 2023
@bors bors bot closed this Aug 10, 2023
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

sigp#4538 

## Proposed Changes

add newtype wrapper around DialError that extracts error messages and logs them in a more readable format

## Additional Info

I was able to test Transport Dial Errors in the situation where a libp2p instance attempts to ping a nonexistent peer. That error message should look something like

`A transport level error has ocurred: Connection refused (os error 61)`

AgeManning mentioned we should try fetching only the most inner error (in situations where theres a nested error). I took a stab at implementing that

For non transport DialErrors, I wrote out the error messages explicitly (as per the docs). Could potentially clean things up here if thats not necessary


Co-authored-by: Age Manning <[email protected]>
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

sigp#4538 

## Proposed Changes

add newtype wrapper around DialError that extracts error messages and logs them in a more readable format

## Additional Info

I was able to test Transport Dial Errors in the situation where a libp2p instance attempts to ping a nonexistent peer. That error message should look something like

`A transport level error has ocurred: Connection refused (os error 61)`

AgeManning mentioned we should try fetching only the most inner error (in situations where theres a nested error). I took a stab at implementing that

For non transport DialErrors, I wrote out the error messages explicitly (as per the docs). Could potentially clean things up here if thats not necessary


Co-authored-by: Age Manning <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants