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

[connection] do not open the connection on initialization #214

Merged
merged 7 commits into from
Oct 20, 2021

Conversation

remeh
Copy link
Contributor

@remeh remeh commented Oct 19, 2021

A recent change clarifying the connection code introduced in v5.3.0 has changed when the socket are opened for both UDP and UDS. Because of that, resolution of an unreachable domain or IP or an non-existing UDS socket made the instantiation of the client throw an error in v5.3.0 since it was happening in the '#initialize' methods, where these errors are not caught.

This PR changes when the socket are opened (on write) where the connection errors are caught by the retry mechanism like it was before v5.3.0.

Should address #213

@remeh remeh added this to the 5.3.1 milestone Oct 19, 2021
lib/datadog/statsd/connection.rb Outdated Show resolved Hide resolved
Co-authored-by: Olivier Vielpeau <olivielpeau@users.noreply.github.com>
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

LGTM 👍 . Just left a few comment wording and test improvement suggestions.

lib/datadog/statsd/connection.rb Outdated Show resolved Hide resolved
Comment on lines 17 to 18
logger.debug { "Statsd: #{payload}" } if logger

Copy link
Member

Choose a reason for hiding this comment

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

⬇️ GitHub doesn't let me comment on line 34, but it appears to me that if we always try to connect before send_message, then the Connection class no longer even needs to know about #connect: it effectively becomes a private implementation detail of the subclasses.

The remaining contract we have is that we can call close and then expect a subsequent send_message to still work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but, I like the clarity of https://github.com/DataDog/dogstatsd-ruby/pull/214/files#diff-01cb409d6d5a1fb2aebe52e78e877a91e3f6654758931899427a43656211a073R32-R35
You can explicitly read in the retry mechanism that it is closing the connection, re-opening it, and retrying.
I know the connect isn't needed since send_message will do it.
Not a strong opinion though, I can remove the connect here.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I don't quite like having the mix of implicit (in send_message) and explicit (in the failure handler) but I can live with it :)

lib/datadog/statsd/udp_connection.rb Outdated Show resolved Hide resolved
spec/statsd/udp_connection_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@remeh remeh merged commit 95a7544 into master Oct 20, 2021
@remeh remeh deleted the remeh/connection-on-send branch October 20, 2021 11:57
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.

None yet

3 participants