replace servo with spectator #1526
Conversation
| @@ -87,11 +86,11 @@ private EurekaMonitors(String name, String description) { | |||
| } | |||
| } | |||
|
|
|||
| @com.netflix.servo.annotations.Monitor(name = "count", type = DataSourceType.COUNTER) | |||
There was a problem hiding this comment.
This seems like a curiously named metric "count", is there something that gets added with the servo Monitor that allows us to disambiguate such a generically named id @brharrington?
Looks like there is a className dimension for a Monitor
There was a problem hiding this comment.
It would have a dimenion class=EurekaMonitors. See: https://github.com/Netflix/servo/blob/master/servo-core/src/main/java/com/netflix/servo/monitor/Monitors.java#L264
There was a problem hiding this comment.
Got it, should I add this dimension to the metrics? Will there be any gotchas about emitting that dimension manually?
There was a problem hiding this comment.
For better compatibility with the previous data it would be good to add it explicitly. There shouldn't be any issues with adding it directly.
There was a problem hiding this comment.
Will go ahead and do that to attempt to avoid any external changes in the metrics. Thanks @brharrington
| .withPublishStdDev(true) | ||
| .build(); | ||
| final MonitorConfig config = MonitorConfig.builder(METRIC_REPLICATION_PREFIX + "batchSize").build(); | ||
| this.batchSizeMetric = new StatsTimer(config, statsConfig); |
There was a problem hiding this comment.
I wasn't sure what to do with StatsTimer and the docs just say it's "not supported". Seems like PercentileTimer is a decent replacement
https://netflix.github.io/atlas-docs/spectator/lang/java/servo-migration/#statstimer
There was a problem hiding this comment.
If percentiles are really needed, then PercentileTimer or PercentileDistributionSummary would need to be used. Either way, it wouldn't be fully backwards compatible, so the user may need to tune their queries.
I'm not sure sure how this metric is used. The name is batchSize which doesn't seem like it would be measuring time, so PercentileDistributionSummary may be more appropriate. It was using a timer though.
There was a problem hiding this comment.
Changed this one to a PercentileDistributionSummary but left another one as percentiletimer but noted that something is going to break if people are using this metric
| @@ -10,7 +10,8 @@ dependencies { | |||
| api "com.amazonaws:aws-java-sdk-route53:${awsVersion}" | |||
| api "jakarta.servlet:jakarta.servlet-api:${servletVersion}" | |||
| api 'jakarta.inject:jakarta.inject-api:2.0.1' | |||
| api 'com.thoughtworks.xstream:xstream:1.4.19' | |||
| api 'com.thoughtworks.xstream:xstream:1.4.20' | |||
| implementation 'com.google.guava:guava:33.0.0-jre' | |||
There was a problem hiding this comment.
Since this PR is about servo->spectator I'm leaving the guava dependency in place (but updating to the latest version), I can follow up with another PR that will replace the guava usages
eureka-client/build.gradle
Outdated
| api "com.netflix.servo:servo-core:${servoVersion}" | ||
| api "com.netflix.spectator:spectator-api:${spectatorVersion}" | ||
| api "org.slf4j:slf4j-api:${slf4jVersion}" | ||
| implementation "com.netflix.netflix-commons:netflix-eventbus:0.3.0" |
There was a problem hiding this comment.
netflix-commons is also legacy stuff that brings in dependencies that can be problematic, e.g. servlet-api.
There was a problem hiding this comment.
That looks like an issue with the cherry pick from my old commit, this is already present in the build.gradle, I agree netflix-common is not ideal but for now we'll leave that for future pull request 😄
There was a problem hiding this comment.
I changed it from api->implementation which should help a little bit for now
| @@ -87,11 +86,11 @@ private EurekaMonitors(String name, String description) { | |||
| } | |||
| } | |||
|
|
|||
| @com.netflix.servo.annotations.Monitor(name = "count", type = DataSourceType.COUNTER) | |||
There was a problem hiding this comment.
It would have a dimenion class=EurekaMonitors. See: https://github.com/Netflix/servo/blob/master/servo-core/src/main/java/com/netflix/servo/monitor/Monitors.java#L264
| .withPublishStdDev(true) | ||
| .build(); | ||
| final MonitorConfig config = MonitorConfig.builder(METRIC_REPLICATION_PREFIX + "batchSize").build(); | ||
| this.batchSizeMetric = new StatsTimer(config, statsConfig); |
There was a problem hiding this comment.
If percentiles are really needed, then PercentileTimer or PercentileDistributionSummary would need to be used. Either way, it wouldn't be fully backwards compatible, so the user may need to tune their queries.
I'm not sure sure how this metric is used. The name is batchSize which doesn't seem like it would be measuring time, so PercentileDistributionSummary may be more appropriate. It was using a timer though.
joshgord
force-pushed
the
josh/guava2x
branch
3 times, most recently
from
9dd4af4 to
8517b46
Compare
|
|
||
| private ServoUtil() { | ||
| } | ||
| public static <T> T monitoredValue(@Nonnull String name, @Nonnull T obj, |
There was a problem hiding this comment.
Added this helper value that adds the class dimension to the metric by default
joshgord
force-pushed
the
josh/guava2x
branch
3 times, most recently
from
59c157c to
3a4d9f9
Compare
This is a port of this PR to the 2.x branch #1523
In general when trivial I changed direct usages of guava with their replacements. I did leave guava in the eureka-core project because we'll have to reimplement some caches.