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

Reduce DNS lookups for the statsd domain #104

Merged
merged 1 commit into from
Jul 24, 2017
Merged

Conversation

elukey
Copy link
Contributor

@elukey elukey commented Jul 24, 2017

The sendto socket call is very flexible but it currently
issue a DNS lookup for the statsd domain for every
metric in the for loop to push. This change ensures that
only one lookup is performed for each batch of metrics
to send. It is particularly useful when logster runs
every minute and the number of metrics to push is very high.

Issue: #103

The sendto socket call is very flexible but it currently
issue a DNS lookup for the statsd domain for every
metric in the for loop to push. This change ensures that
only one lookup is performed for each batch of metrics
to send. It is particularly useful when logster runs
every minute and the number of metrics to push is very high.
@benburry
Copy link
Contributor

Thanks for your PR. Could you make the same change (resolve name -> ip before socket connect) to the other output module that follow this pattern, graphite, so that we can maintain consistent behaviour across them?

@elukey
Copy link
Contributor Author

elukey commented Jul 24, 2017

I think that the graphite module uses a different pattern, namely s.connect((host[0], int(host[1]))) before the loop and then s.sendall (that afaict it should not generate a DNS query). The statsd module creates a socket + sendto() for each iteration.

@benburry
Copy link
Contributor

So it does. Thanks!

@benburry benburry merged commit a242498 into etsy:master Jul 24, 2017
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.

2 participants