Avoid host lookups if trace tags have already been provided #371
Conversation
Codecov Report
@@ Coverage Diff @@
## master #371 +/- ##
============================================
+ Coverage 84.5% 84.72% +0.21%
- Complexity 619 624 +5
============================================
Files 95 95
Lines 2459 2468 +9
Branches 274 276 +2
============================================
+ Hits 2078 2091 +13
+ Misses 284 283 -1
+ Partials 97 94 -3
Continue to review full report at Codecov.
|
| String hostname = getHostName(); | ||
| if (hostname != null) { | ||
| tags.put(Constants.TRACER_HOSTNAME_TAG_KEY, hostname); | ||
| Object hostNameTag = tags.get(Constants.TRACER_HOSTNAME_TAG_KEY); |
There was a problem hiding this comment.
use if (tags.containsKey(Constants.TRACER_HOSTNAME_TAG_KEY)) to avoid leaking variable name to outer scope
There was a problem hiding this comment.
That’s me being consistent with the code below. I will modify to your tastes.
| Object ipTag = tags.get(Constants.TRACER_IP_TAG_KEY); | ||
| if (ipTag == null) { | ||
| try { | ||
| tags.put(Constants.TRACER_IP_TAG_KEY, InetAddress.getLocalHost().getHostAddress()); |
There was a problem hiding this comment.
can DRY InetAddress.getLocalHost().getHostAddress()
There was a problem hiding this comment.
Sorry, where am I repeating myself?
There was a problem hiding this comment.
sorry, nvm, the 2nd one is getting v4 address
huntc
force-pushed
the
avoid-host-lookup
branch
from
4022908
to
e673f43
Compare
huntc
changed the title
|
@yurishkuro PR updated, signed off and tests included. |
huntc
force-pushed
the
avoid-host-lookup
branch
from
e673f43
to
b108815
Compare
Signed-off-by: Christopher Hunt <huntchr@gmail.com>
huntc
force-pushed
the
avoid-host-lookup
branch
from
b108815
to
8f90229
Compare
|
I don't know what you want to do with this - I'm still not hitting the test coverage, despite there being no test for the specific hostname resolution failing before. I don't know how to make that bit of code fail either. I'm pretty confident that the changes I've introduced are covered well with the tests. TBH, for such a small PR, I've spent about 4 hours, so any further assistance would be appreciated. |
|
Thanks! The test coverage changes is sometimes indeed confusing.
Do you feel this is due to the way this repo is setup, or some other reasons? Do you have suggestions on what we can improved? |
It was probably the test coverage thing mostly. But then, I’ve not been subject to Cobertura for some time so may be it is just me! I’m left feeling positive that Jaegertracing has a strong focus on quality which certainly matches my experience of using it. |
This matches the functionality of jaeger-client-java after jaegertracing/jaeger-client-java#371 was merged. Refs: jaegertracing#166
This matches the functionality of jaeger-client-java after jaegertracing/jaeger-client-java#371 was merged. Refs: jaegertracing#166 Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org>
* Allow specifying the hostname via a tag This is useful for applications running inside containers where the hostname is autogenerated and generally useless. In kubernetes, the downward api can be used to pass in the node name actually running the container. Fixes #66 Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org> * Catch the `socket.error` exception when trying to get the hostname This is the base exception for all errors in the socket library. Explicit is better than implicit! Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org> * Don't attempt to lookup the ip address if specified via a tag This matches the functionality of jaeger-client-java after jaegertracing/jaeger-client-java#371 was merged. Refs: #166 Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org> * Remove ipv4_to_int since jaeger converts tags to strings Signed-off-by: Jeff Schroeder <jeffschroeder@computer.org>
Fixes jaegertracing/jaeger#742 (comment)
Marked as WIP as I've not yet tested it etc.