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

fix: error handling and logging #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daveads
Copy link

@daveads daveads commented Dec 13, 2024

fix: improve error handling and logging in EventBroadcaster

  • Add dedicated connection method with proper error handling and logging
  • Improve error visibility by logging connection failures at ERROR level
  • Add more descriptive error messages including problematic URLs

This improves visibility of broadcast connection issues like DNS resolution failures
that were previously being swallowed. Fixes issue where typos in broadcast URIs
could go unnoticed due to insufficient error logging.

permitio/opal#716

@orweis
Copy link
Contributor

orweis commented Dec 16, 2024

Hi @daveads - thanks for your efforts here - much appreciated.
While the code seems okay - I don't think this is the right way to approach permitio/opal#716 and permitio/opal#711 ;
As the issue here isn't that this library (fastapi_websocket_pubsub) is swallowing the errors- but that the OPAL code using it is / is using the wrong log level for communicating them.

In addition the tests are currently hanging / failing.
I'd advise closing this PR and addressing the issues in OPAL itself

CC: @gemanor , @danyi1212

@daveads
Copy link
Author

daveads commented Dec 16, 2024

Hi @daveads - thanks for your efforts here - much appreciated. While the code seems okay - I don't think this is the right way to approach permitio/opal#716 and permitio/opal#711 ; As the issue here isn't that this library (fastapi_websocket_pubsub) is swallowing the errors- but that the OPAL code using it is / is using the wrong log level for communicating them.

In addition the tests are currently hanging / failing. I'd advise closing this PR and addressing the issues in OPAL itself

CC: @gemanor , @danyi1212

I’ll check this out later to debug further. In the meantime, the PR can be closed.

@daveads
Copy link
Author

daveads commented Dec 16, 2024

@orweis, could you run these tests locally? I found out they were failing on the master branch as well, not just in this PR.

@orweis
Copy link
Contributor

orweis commented Dec 16, 2024

In that case I'd need to ask @obsd and @danyi1212 to look at them

@daveads
Copy link
Author

daveads commented Dec 16, 2024

In that case I'd need to ask @obsd and @danyi1212 to look at them

sure...

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.

2 participants