-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[internal/rabbitmq] move connection and retry logic into separate pkg #34361
Conversation
cabf4a8
to
730f926
Compare
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
730f926
to
55d0786
Compare
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
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.
looks good! just added a couple minor comments
@@ -307,15 +316,20 @@ type mockConnection struct { | |||
mock.Mock | |||
} | |||
|
|||
func (m *mockConnection) ReconnectIfUnhealthy() error { |
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.
looks like there's no longer unit test coverage for the actual implementation of this method? might be worth adding a unit test for it in the internal package
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.
Agree. I added a test in the internal/rabbitmq
pkg.
"time" | ||
|
||
amqp "github.com/rabbitmq/amqp091-go" | ||
"go.uber.org/zap" | ||
|
||
"github.com/open-telemetry/opentelemetry-collector-contrib/internal/rabbitmq" |
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.
maybe it's worth aliasing this import so its clearer when skimming the code that rabbitmq
refers to an otel-contrib package and not the external rabbitmq client library
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.
otelrabbitmq
?
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.
Changed it 👍
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
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.
LGTM. @atoulme, do mind taking a look when you get a chance?
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.
LGTM!
FTR: just resolved merge conflicts in the |
Resolve conflicts in go.mod Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>
78cc97d
to
af259d8
Compare
…open-telemetry#34361) This PR moves the retry logic from amqp publisher to amqp connection. Connection, client and other utility structures have been moved from `exporter/rabbitmqexporter/internal/publisher` to `internal/rabbitmq`. **Link to tracking Issue:** open-telemetry#34242 ---- cc @swar8080 @atoulme --------- Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de> Co-authored-by: Alex Boten <223565+codeboten@users.noreply.github.com>
Description:
This PR moves the retry logic from amqp publisher to amqp connection. Connection, client and other utility structures have been moved from
exporter/rabbitmqexporter/internal/publisher
tointernal/rabbitmq
.Link to tracking Issue:
Testing:
Documentation:
cc @swar8080 @atoulme