replace servo with spectator in the client #1523
Conversation
joshgord
force-pushed
the
josh/servo
branch
2 times, most recently
from
69dd733
to
291ad28
Compare
| @@ -67,6 +69,7 @@ public class TimeConsumingInstanceRegistryTest extends AbstractTester { | |||
| * Note that there is a thread retrieving and printing out registry status for debugging purpose. | |||
| */ | |||
| @Test | |||
| @Ignore("NumOfRenewsPerMinThreshold should be updated to 256") | |||
There was a problem hiding this comment.
Was already failing in the master branch
| @@ -27,6 +27,8 @@ dependencies { | |||
| compile project(':eureka-client') | |||
| compile 'org.glassfish.jersey.core:jersey-client:2.23.1' | |||
| compile 'org.glassfish.jersey.connectors:jersey-apache-connector:2.23.1' | |||
| compile "org.slf4j:slf4j-api:1.7.36" | |||
There was a problem hiding this comment.
slf4j apparently pulled in transitively via servo
| @@ -27,6 +27,8 @@ dependencies { | |||
| compile project(':eureka-client') | |||
| compile 'org.glassfish.jersey.core:jersey-client:2.23.1' | |||
| compile 'org.glassfish.jersey.connectors:jersey-apache-connector:2.23.1' | |||
| compile "org.slf4j:slf4j-api:1.7.36" | |||
| compile 'com.netflix.spectator:spectator-api:1.7.3' | |||
There was a problem hiding this comment.
Should it also use ${spectatorVersion} here?
There was a problem hiding this comment.
Seems like this sub project didn't adopt the version substitution like the others 🤷♂️
There was a problem hiding this comment.
May just need to use double quotes
| } catch (Exception e) { | ||
| s_logger.error("Unable to register with servo.", e); | ||
| } | ||
| final CompositeRegistry registry = Spectator.globalRegistry(); |
There was a problem hiding this comment.
You should be able to just use the base Registry type for instrumentation. It makes it easier to swap out which implementation is used later if needed.
There was a problem hiding this comment.
Ah right, this was my IDE autocompleting the type, will update
| final CompositeRegistry registry = globalRegistry(); | ||
| PolledMeter.using(registry) | ||
| .withName(METRIC_REGISTRY_PREFIX + "lastSuccessfulRegistryFetchTimePeriod") | ||
| .monitorValue(getLastSuccessfulRegistryFetchTimePeriodInternal()); |
There was a problem hiding this comment.
As written it would just report the initial value. The method that takes a Number is meant for things like AtomicInteger that would get updated over time. Unfortunately though, primitive types are implicitly cast so this compiles. Pass in the lambda so it can be invoked each time to get the updated value. For example:
.monitorValue(this, DiscoveryClient::getLastSuccessfulRegistryFetchTimePeriodInternal)There was a problem hiding this comment.
Ah good catch. I was going to ping you to see if I managed to convert the monitors correctly, will make this change 👍
|
|
||
| if (config.shouldRegisterWithEureka()) { | ||
| this.heartbeatStalenessMonitor = new ThresholdLevelsMetric(this, METRIC_REGISTRATION_PREFIX + "lastHeartbeatSec_", new long[]{15L, 30L, 60L, 120L, 240L, 480L}); | ||
| } else { | ||
| this.heartbeatStalenessMonitor = ThresholdLevelsMetric.NO_OP_METRIC; | ||
| } | ||
| PolledMeter.using(registry) | ||
| .withName(METRIC_REGISTRATION_PREFIX + "lastSuccessfulHeartbeatTimePeriod") | ||
| .monitorValue(getLastSuccessfulHeartbeatTimePeriodInternal()); |
There was a problem hiding this comment.
See note on using lambda
| @@ -387,6 +395,9 @@ public synchronized BackupRegistry get() { | |||
| initTimestampMs = System.currentTimeMillis(); | |||
| initRegistrySize = this.getApplications().size(); | |||
| registrySize = initRegistrySize; | |||
| PolledMeter.using(registry) | |||
| .withName(METRIC_REGISTRY_PREFIX + "localRegistrySize") | |||
| .monitorValue(registrySize); | |||
There was a problem hiding this comment.
See note on using lambda
| final CompositeRegistry registry = Spectator.globalRegistry(); | ||
| PolledMeter.using(registry) | ||
| .withName(METRIC_RESOLVER_PREFIX + "lastLoadTimestamp") | ||
| .monitorValue(getLastLoadTimestamp()); |
There was a problem hiding this comment.
See note on using lambda
| Monitors.registerObject(name, this); | ||
| PolledMeter.using(registry) | ||
| .withName(METRIC_RESOLVER_PREFIX + "endpointsSize") | ||
| .monitorValue(getEndpointsSize()); |
There was a problem hiding this comment.
See note on using lambda
| } | ||
| PolledMeter.using(Spectator.globalRegistry()) | ||
| .withName(METRIC_RESOLVER_PREFIX + "lastReloadTimestamp") | ||
| .monitorValue(getLastReloadTimestamp()); |
There was a problem hiding this comment.
See note on using lambda
| Monitors.registerObject(name, this); | ||
| PolledMeter.using(Spectator.globalRegistry()) | ||
| .withName(METRIC_TRANSPORT_PREFIX + "currentSessionDuration") | ||
| .monitorValue(getCurrentSessionDuration()); |
There was a problem hiding this comment.
See note on using lambda
| } | ||
| final CompositeRegistry registry = Spectator.globalRegistry(); | ||
| executionTimeStats = registry.timer("Eureka-Connection-Cleaner-Time"); | ||
| cleanupFailed = registry.counter("Eureka-Connection-Cleaner-Failure"); |
| @@ -27,6 +27,8 @@ dependencies { | |||
| compile project(':eureka-client') | |||
| compile 'org.glassfish.jersey.core:jersey-client:2.23.1' | |||
| compile 'org.glassfish.jersey.connectors:jersey-apache-connector:2.23.1' | |||
| compile "org.slf4j:slf4j-api:1.7.36" | |||
| compile 'com.netflix.spectator:spectator-api:1.7.3' | |||
There was a problem hiding this comment.
May just need to use double quotes
No description provided.