-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
replace servo with spectator in the client #1523
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it also use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this sub project didn't adopt the version substitution like the others 🤷♂️ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May just need to use double quotes |
||
|
||
testCompile (project(':eureka-test-utils')) { | ||
// exclude all transitives to avoid bringing in jersey1 eureka-core | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,11 @@ | |
|
||
import static com.netflix.discovery.util.DiscoveryBuildInfo.buildVersion; | ||
|
||
import com.netflix.discovery.util.ServoUtil; | ||
import com.netflix.spectator.api.CompositeRegistry; | ||
import com.netflix.spectator.api.Counter; | ||
import com.netflix.spectator.api.Spectator; | ||
import com.netflix.spectator.api.Timer; | ||
import java.io.FileInputStream; | ||
import java.io.IOException; | ||
import java.security.KeyStore; | ||
|
@@ -36,12 +41,6 @@ | |
import com.netflix.discovery.converters.wrappers.DecoderWrapper; | ||
import com.netflix.discovery.converters.wrappers.EncoderWrapper; | ||
import com.netflix.discovery.provider.DiscoveryJerseyProvider; | ||
import com.netflix.servo.monitor.BasicCounter; | ||
import com.netflix.servo.monitor.BasicTimer; | ||
import com.netflix.servo.monitor.Counter; | ||
import com.netflix.servo.monitor.MonitorConfig; | ||
import com.netflix.servo.monitor.Monitors; | ||
import com.netflix.servo.monitor.Stopwatch; | ||
|
||
/** | ||
* @author Tomasz Bak | ||
|
@@ -325,24 +324,19 @@ private PoolingHttpClientConnectionManager createCustomSslCM() { | |
private class ConnectionCleanerTask implements Runnable { | ||
|
||
private final int connectionIdleTimeout; | ||
private final BasicTimer executionTimeStats; | ||
private final Timer executionTimeStats; | ||
private final Counter cleanupFailed; | ||
|
||
private ConnectionCleanerTask(int connectionIdleTimeout) { | ||
this.connectionIdleTimeout = connectionIdleTimeout; | ||
MonitorConfig.Builder monitorConfigBuilder = MonitorConfig.builder("Eureka-Connection-Cleaner-Time"); | ||
executionTimeStats = new BasicTimer(monitorConfigBuilder.build()); | ||
cleanupFailed = new BasicCounter(MonitorConfig.builder("Eureka-Connection-Cleaner-Failure").build()); | ||
try { | ||
Monitors.registerObject(this); | ||
} catch (Exception e) { | ||
s_logger.error("Unable to register with servo.", e); | ||
} | ||
final CompositeRegistry registry = Spectator.globalRegistry(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to just use the base There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right, this was my IDE autocompleting the type, will update There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
executionTimeStats = registry.timer("Eureka-Connection-Cleaner-Time"); | ||
cleanupFailed = registry.counter("Eureka-Connection-Cleaner-Failure"); | ||
} | ||
|
||
@Override | ||
public void run() { | ||
Stopwatch start = executionTimeStats.start(); | ||
long monotonicTime = ServoUtil.time(executionTimeStats); | ||
try { | ||
HttpClientConnectionManager cm = (HttpClientConnectionManager) apacheHttpClient | ||
.getConfiguration() | ||
|
@@ -352,11 +346,8 @@ public void run() { | |
s_logger.error("Cannot clean connections", e); | ||
cleanupFailed.increment(); | ||
} finally { | ||
if (null != start) { | ||
start.stop(); | ||
} | ||
ServoUtil.record(executionTimeStats, monotonicTime); | ||
} | ||
|
||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,13 @@ | |
|
||
import static com.netflix.discovery.EurekaClientNames.METRIC_REGISTRATION_PREFIX; | ||
import static com.netflix.discovery.EurekaClientNames.METRIC_REGISTRY_PREFIX; | ||
import static com.netflix.spectator.api.Spectator.globalRegistry; | ||
|
||
import com.netflix.discovery.util.ServoUtil; | ||
import com.netflix.spectator.api.CompositeRegistry; | ||
import com.netflix.spectator.api.Counter; | ||
import com.netflix.spectator.api.Timer; | ||
import com.netflix.spectator.api.patterns.PolledMeter; | ||
import java.util.ArrayList; | ||
import java.util.Collection; | ||
import java.util.Collections; | ||
|
@@ -83,10 +89,6 @@ | |
import com.netflix.discovery.shared.transport.jersey.Jersey1TransportClientFactories; | ||
import com.netflix.discovery.shared.transport.jersey.TransportClientFactories; | ||
import com.netflix.discovery.util.ThresholdLevelsMetric; | ||
import com.netflix.servo.annotations.DataSourceType; | ||
import com.netflix.servo.monitor.Counter; | ||
import com.netflix.servo.monitor.Monitors; | ||
import com.netflix.servo.monitor.Stopwatch; | ||
|
||
/** | ||
* The class that is instrumental for interactions with <tt>Eureka Server</tt>. | ||
|
@@ -130,11 +132,10 @@ public class DiscoveryClient implements EurekaClient { | |
|
||
// Timers | ||
private static final String PREFIX = "DiscoveryClient_"; | ||
private final Counter RECONCILE_HASH_CODES_MISMATCH = Monitors.newCounter(PREFIX + "ReconcileHashCodeMismatch"); | ||
private final com.netflix.servo.monitor.Timer FETCH_REGISTRY_TIMER = Monitors | ||
.newTimer(PREFIX + "FetchRegistry"); | ||
private final Counter REREGISTER_COUNTER = Monitors.newCounter(PREFIX | ||
+ "Reregister"); | ||
private final Counter RECONCILE_HASH_CODES_MISMATCH = globalRegistry().counter(PREFIX + "ReconcileHashCodeMismatch"); | ||
private final Timer FETCH_REGISTRY_TIMER = globalRegistry().timer(PREFIX + "FetchRegistry"); | ||
private final Counter REREGISTER_COUNTER = globalRegistry().counter(PREFIX | ||
+ "Reregister"); | ||
|
||
// instance variables | ||
/** | ||
|
@@ -362,12 +363,19 @@ public synchronized BackupRegistry get() { | |
} else { | ||
this.registryStalenessMonitor = ThresholdLevelsMetric.NO_OP_METRIC; | ||
} | ||
final CompositeRegistry registry = globalRegistry(); | ||
PolledMeter.using(registry) | ||
.withName(METRIC_REGISTRY_PREFIX + "lastSuccessfulRegistryFetchTimePeriod") | ||
.monitorValue(getLastSuccessfulRegistryFetchTimePeriodInternal()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As written it would just report the initial value. The method that takes a .monitorValue(this, DiscoveryClient::getLastSuccessfulRegistryFetchTimePeriodInternal) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good catch. I was going to ping you to see if I managed to convert the monitors correctly, will make this change 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See note on using lambda |
||
|
||
logger.info("Initializing Eureka in region {}", clientConfig.getRegion()); | ||
|
||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See note on using lambda |
||
logger.info("Discovery Client initialized at timestamp {} with initial instances count: {}", | ||
initTimestampMs, initRegistrySize); | ||
|
||
|
@@ -475,12 +486,6 @@ public synchronized BackupRegistry get() { | |
// finally, init the schedule tasks (e.g. cluster resolvers, heartbeat, instanceInfo replicator, fetch | ||
initScheduledTasks(); | ||
|
||
try { | ||
Monitors.registerObject(this); | ||
} catch (Throwable e) { | ||
logger.warn("Cannot register timers", e); | ||
} | ||
|
||
// This is a bit of hack to allow for existing code using DiscoveryManager.getInstance() | ||
// to work with DI'd DiscoveryClient | ||
DiscoveryManager.getInstance().setDiscoveryClient(this); | ||
|
@@ -952,11 +957,6 @@ public synchronized void shutdown() { | |
eurekaTransport.shutdown(); | ||
} | ||
|
||
heartbeatStalenessMonitor.shutdown(); | ||
registryStalenessMonitor.shutdown(); | ||
|
||
Monitors.unregisterObject(this); | ||
|
||
logger.info("Completed shut down of DiscoveryClient"); | ||
} | ||
} | ||
|
@@ -990,7 +990,7 @@ void unregister() { | |
* @return true if the registry was fetched | ||
*/ | ||
private boolean fetchRegistry(boolean forceFullRegistryFetch) { | ||
Stopwatch tracer = FETCH_REGISTRY_TIMER.start(); | ||
long monotonicTime = ServoUtil.time(FETCH_REGISTRY_TIMER); | ||
|
||
try { | ||
// If the delta is disabled or if it is the first time, get all | ||
|
@@ -1022,9 +1022,7 @@ private boolean fetchRegistry(boolean forceFullRegistryFetch) { | |
appPathIdentifier, clientConfig.getRegistryFetchIntervalSeconds(), e.getMessage(), ExceptionUtils.getStackTrace(e)); | ||
return false; | ||
} finally { | ||
if (tracer != null) { | ||
tracer.stop(); | ||
} | ||
ServoUtil.record(FETCH_REGISTRY_TIMER, monotonicTime); | ||
} | ||
|
||
// Notify about cache refresh before updating the instance remote status | ||
|
@@ -1730,8 +1728,6 @@ public long getLastSuccessfulRegistryFetchTimePeriod() { | |
: System.currentTimeMillis() - lastSuccessfulRegistryFetchTimestamp; | ||
} | ||
|
||
@com.netflix.servo.annotations.Monitor(name = METRIC_REGISTRATION_PREFIX + "lastSuccessfulHeartbeatTimePeriod", | ||
description = "How much time has passed from last successful heartbeat", type = DataSourceType.GAUGE) | ||
private long getLastSuccessfulHeartbeatTimePeriodInternal() { | ||
final long delay = (!clientConfig.shouldRegisterWithEureka() || isShutdown.get()) | ||
? 0 | ||
|
@@ -1742,8 +1738,6 @@ private long getLastSuccessfulHeartbeatTimePeriodInternal() { | |
} | ||
|
||
// for metrics only | ||
@com.netflix.servo.annotations.Monitor(name = METRIC_REGISTRY_PREFIX + "lastSuccessfulRegistryFetchTimePeriod", | ||
description = "How much time has passed from last successful local registry update", type = DataSourceType.GAUGE) | ||
private long getLastSuccessfulRegistryFetchTimePeriodInternal() { | ||
final long delay = (!clientConfig.shouldFetchRegistry() || isShutdown.get()) | ||
? 0 | ||
|
@@ -1753,13 +1747,6 @@ private long getLastSuccessfulRegistryFetchTimePeriodInternal() { | |
return delay; | ||
} | ||
|
||
@com.netflix.servo.annotations.Monitor(name = METRIC_REGISTRY_PREFIX + "localRegistrySize", | ||
description = "Count of instances in the local registry", type = DataSourceType.GAUGE) | ||
public int localRegistrySize() { | ||
return registrySize; | ||
} | ||
|
||
|
||
private long computeStalenessMonitorDelay(long delay) { | ||
if (delay < 0) { | ||
return System.currentTimeMillis() - initTimestampMs; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slf4j apparently pulled in transitively via servo