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

Set localhost if gethostbyname() is un-resolvable #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

geek876
Copy link

@geek876 geek876 commented Nov 19, 2017

… the 'host' can't be resolved either via DNS or via local-hosts file. Setting 'localhost' under such cases.

… the 'host' can't be resolved either via DNS or via local-hosts file. Setting 'localhost' under such cases.
@geek876 geek876 mentioned this pull request Nov 19, 2017
@codecov-io
Copy link

codecov-io commented Nov 19, 2017

Codecov Report

Merging #34 into master will decrease coverage by 0.96%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
- Coverage   82.78%   81.81%   -0.97%     
==========================================
  Files           2        2              
  Lines         151      154       +3     
  Branches       18       18              
==========================================
+ Hits          125      126       +1     
- Misses         22       24       +2     
  Partials        4        4
Impacted Files Coverage Δ
cmreslogging/handlers.py 80.95% <50%> (-1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6765a24...1539d2c. Read the comment docs.

@cmanaha
Copy link
Owner

cmanaha commented Nov 20, 2017

Appreciate the pull request, but I'm not entirely sure I'm comfortable adding this to master for the following reason:

  • The fix basically sets a default to localhost with ip 127.0.0.1 if we cannot resolve the name of the current host... If we've come to this point is because the host network has been poorly configured or has been misconfigured somehow.
  • Instead to fix the root cause (make the system resolve properly the current host) the PR basically sets a default to localhost with ip 127.0.0.1... So for a distributed logging system we end up reporting that logs are coming from 127.0.0.1/localhost which will be misleading too, at least from the point of view of identifying which of the nodes reporting logs in a distributed network has issues.

Rather than adding a default source that might be misleading and not addressing the problem, I think I'd rather prefer a behaviour that forces a fail fast and points out what the problem is and how to fix it so users can remediate root problems at source.

@cmanaha cmanaha self-requested a review November 20, 2017 23:32
Copy link
Owner

@cmanaha cmanaha left a comment

Choose a reason for hiding this comment

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

Read comment on addressing problem at source. As stated in my comment I'm not convinced that adding a default when network has the wrong configuration is the right thing to do here. I'd rather prefer to address the root problem by using a fail fast approach and helping the user to understand what the issue is and pointing him in a direction on how to solve it, for example.

@dougiep16
Copy link

Ran into a similar problem today, and came across this SO answer for determining your IP address.

https://stackoverflow.com/a/28950776

This worked for me, even though socket.gethostbyname(socket.gethostname()) threw an exception.

I completely agree with the fail fast approach, however, I think we can add some robustness to the library if we develop a simple function that runs through a variety a different methods and then ultimately fails if none are successful.

@cmanaha
Copy link
Owner

cmanaha commented Nov 21, 2017

I think we can add some robustness to the library if we develop a simple function that runs through a variety a different methods and then ultimately fails if none are successful.

@dougiep16: That makes sense to me as well.

@yamadmia: If you opt for this approach, l'd suggest considering moving that part of the code (finding out info on the current running host) to a different file so we try to keep the handler clean. Most of other methods to find out the IP address might be platform dependent. Whatever we do, we must ensure the handler remains platform independent.

@zigius zigius mentioned this pull request Jan 1, 2018
@wotori
Copy link

wotori commented Nov 18, 2022

seems still reasonable for mac users

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.

5 participants