-
Notifications
You must be signed in to change notification settings - Fork 140
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
Support IP changes of statsd hostname #284
base: master
Are you sure you want to change the base?
Conversation
…p and reconnect the UDP socket in this case
Todo: Disable this by default and make interval configurable |
Looks good to me :-) |
Looks good 👍 |
Hey @Goltergaul Thanks for you contribution. A problem with this implementation is that the A mitigation to your problem which can be considered with the |
my 2 cents, IMHO this should be handled (if enabled) in the statsd library. statsd is often used indirectly by other libraries to report metrics and for example not directly in our applications. Also the other concerns you mention like DNS errors while the old statsd ip is still reachable can be handled with some added error handling. |
@remeh Thanks for your comment. While your proposed mitigation works I would expect from such a library that it can reconnect itself automatically in such a case, as library user I don't want to take care of this myself. As for the problems: As for speed considerations when the dns resolution is slow. Maybe add a low timeout, e.g.
This would mean the thread is blocked at max 100ms every minute (not sure if that is enough though). Also this behavior could be made configurable with custom timeouts or even disable it completely. Additionally i assume the dns lookup via resolv should also profit from some low level dns caching? Someone here knows more about this? At least it is pretty fast if I try to resolve 10000 times the same domain Or as suggested above, we could make it async with a Fiber or so (sounds like the most robust one) I'm not an expert on Fibers, but I think this will work only with ruby 3. |
f922b42
to
9378e6d
Compare
we have the issue that the ip address of our statsd host is changing regularly on our kubernetes cluster due to some automated updates. In this case the current implementation continues to send UDP packets to the old IP address.
This pr addresses this issue by resolving the hostname to its ip all 60 seconds. in case the ip changed, it simply reconnects the UDP socket.
This is a bit of a draft to get your input. Please let me know if this is something you would be willing to merge
FYI this currently failes the allocation specs as I did not adapt those yet