python-elasticsearch-logger
python-elasticsearch-logger copied to clipboard
Set localhost if gethostbyname() is un-resolvable
… the 'host' can't be resolved either via DNS or via local-hosts file. Setting 'localhost' under such cases.
Codecov Report
Merging #34 into master will decrease coverage by
0.96%
. The diff coverage is50%
.
@@ 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%) |
:arrow_down: |
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.
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.
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.
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.
seems still reasonable for mac users