-
Notifications
You must be signed in to change notification settings - Fork 299
Added customer connection lost test from issue #2503 #2899
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
base: main
Are you sure you want to change the base?
Added customer connection lost test from issue #2503 #2899
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a test for handling consumer errors when the customer connection is lost and introduces optional AMQP tracing capabilities. The changes enable testing of error scenarios in the Event Hubs consumer client and provide enhanced debugging capabilities through AMQP tracing.
- Added a comprehensive integration test that validates Event Hubs consumer behavior during connection error scenarios
- Introduced an optional
amqp_tracing
feature to enable fe2o3_amqp tracing for debugging purposes
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
sdk/eventhubs/azure_messaging_eventhubs/tests/eventhubs_consumer_error.rs | New integration test that creates a producer/consumer setup and tests error handling during consumer close operations |
sdk/core/azure_core_amqp/Cargo.toml | Added amqp_tracing feature flag to conditionally enable fe2o3-amqp tracing capabilities |
7ecc446
to
001e672
Compare
@@ -40,6 +40,7 @@ tracing-subscriber = { workspace = true, features = ["env-filter"] } | |||
default = ["fe2o3_amqp"] | |||
cplusplus = [] | |||
test = [] | |||
amqp_tracing = ["fe2o3-amqp/tracing"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts come to mind. First and foremost - and I've started taking this stance with various reqwest
options in azure_core
- I don't think we want to start 1) trying to match parity with dependencies' features, and 2) advertise specific dependencies. At least 2 you did, but because fe2o3_amqp
is optional, this should be:
amqp_tracing = ["fe2o3-amqp/tracing"] | |
amqp_tracing = ["fe2o3-amqp?/tracing"] |
If we added support for other backends in the future, you could add them to this single feature.
That said, and considering what I said above, wouldn't it be better that someone just take a dependency on fe2o3-amqp
themselves and add the feature? Resolve 2+ will unify (combine) them. This is the stance I think we want to take with various reqwest
features apart from a few sane options like gzip and deflate support. Maybe even rustls
given the issues that's causing for some customers.
/cc @RickWinter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we go that route, in your crate's Cargo.toml
either add their tracing
feature in your dev-dependencies
for fe2o3-amqp
(a separate import that adds features - I do this elsewhere and it works fine), or if you just want to do it for this one test, you could try declaring a [[test]]
table just for this one file and add required-features = ["fe2o3-amqp/tracing"]
and see if that works; though, I honestly don't know if it will...and kinda doubt it will, but worth a try.
Also enabled the ability to add fe2o3_amqp traces if needed with an AMQP feature.