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

Stop swalling exceptions in eventhub client and also expire those connections when dead #1

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

erewok
Copy link
Member

@erewok erewok commented Jan 2, 2025

The Eventhub producer clients in this library had some confusing logging going on with exceptions that may have been thrown while publishing Eventhub messages.

Problematically, these clients may have been swallowing exceptions!

This is most likely a holdover from our previous usage of this library with own custom loggers.

We have removed the exception-swallowing behavior and also added a check for disconnected clients, so we can mark these as closed. Lastly, I also added methods on the connection pool to count open connections and to expire a particular connection.

In addition, this PR increases test coverage for the eventhub clients (including Managed) to 99%.

Future Work

For a future PR, consider expiring connections for the following Service Bus exceptions:

  • ServiceBusCommunicationError
  • ServiceBusAuthorizationError
  • ServiceBusAuthenticationError
  • ServiceBusConnectionError

All of the above imported from azure.servicebus.exceptions

Copy link

github-actions bot commented Jan 3, 2025

Code Coverage

Package Line Rate Health
. 90%
clients 83%
testing_utils 76%
Summary 83% (809 / 971)

@erewok erewok merged commit 55e09e4 into main Jan 3, 2025
2 checks passed
@erewok erewok deleted the do-not-squash-evhub-exceptions branch January 3, 2025 01:13
@erewok erewok changed the title Pull out confusing stuff with logger and try/excepts in eventhub Stop swalling exceptions in eventhub client and also expire those connections when dead Jan 3, 2025
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