Resolve servername from the localhost address. #1146
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1146 +/- ##
============================================
+ Coverage 75.23% 75.28% +0.04%
- Complexity 1627 1639 +12
============================================
Files 167 168 +1
Lines 5637 5684 +47
Branches 555 557 +2
============================================
+ Hits 4241 4279 +38
- Misses 1144 1150 +6
- Partials 252 255 +3
Continue to review full report at Codecov.
|
sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java
Outdated
Show resolved
Hide resolved
sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void handleCacheUpdateFailure() { | ||
| expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); |
There was a problem hiding this comment.
there's a class called CurrentDateProvider which returns a System.currentTimeMillis() so you could unit test if necessary.
There was a problem hiding this comment.
If it fails to resolve, it'll kick off again after 1 second.
We catch any error here and set it to 1 second again. So basically this can mean we keep spawning a new thread every second. I think in case of error we should just stop running this and just return null.
WHat makes this fail? Are those errors recoverable? is this a reverse DNS query on the local interface IP addresses?
There was a problem hiding this comment.
Failure is unlikely to happen, I can't think of the case when it would actually happen. Code has changed to use single daemon thread so that at least we don't spawn new threads all the time.
| * | ||
| * @see HostnameCache | ||
| */ | ||
| private static final long HOSTNAME_CACHE_DURATION = TimeUnit.HOURS.toMillis(5); |
There was a problem hiding this comment.
is it a common thing to change the server name? 5h seems too little, looks like also something we could have on the options, but not worth it at the moment
There was a problem hiding this comment.
I honestly don't know. This is how it was implemented in 1.x so I believe there was a reason for that.
There was a problem hiding this comment.
I see, makes sense, not sure if it's worth copying blindly tbh, it's a lot of code and complexity for a simple workaround that would be restarting/redeploying the App, of course, it's suboptimal, but a redeploy would happen soon or later right.
thoughts @bruno-garcia ?
sentry/src/main/java/io/sentry/ServerNameResolvingEventProcessor.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private void handleCacheUpdateFailure() { | ||
| expirationTimestamp = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(1); |
There was a problem hiding this comment.
If it fails to resolve, it'll kick off again after 1 second.
We catch any error here and set it to 1 second again. So basically this can mean we keep spawning a new thread every second. I think in case of error we should just stop running this and just return null.
WHat makes this fail? Are those errors recoverable? is this a reverse DNS query on the local interface IP addresses?
|
@bruno-garcia I can't think of scenario where resolving server name would fail, as Btw, this is how it was implemented in 1.x. |
|
It's now disabled for android and turned on by default when just using Sentry dependency. |
📜 Description
Resolve servername from the localhost address.
💡 Motivation and Context
Fixes #1109.
Large piece of code - resolving and caching the server name has been copied from 1.7.x and updated to new standards.
💚 How did you test it?
Unit tests.
📝 Checklist