Change metrics with micrometer in mantis-control-plane #515
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7ue5wu
requested review from
nickmahilani,
jeffchao,
piygoyal,
calvin681,
sundargates,
Andyz26,
hmitnflx,
markcho,
liuml07 and
fdc-ntflx
as code owners
7ue5wu
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
hmitnflx
reviewed
Aug 8, 2023
| @@ -70,7 +70,7 @@ public static HighAvailabilityServices createHAServices(CoreConfiguration config | |||
| } | |||
| else { | |||
| if (HAServiceInstanceRef.get() == null) { | |||
| HAServiceInstanceRef.compareAndSet(null, new ZkHighAvailabilityServices(configuration)); | |||
| HAServiceInstanceRef.compareAndSet(null, new ZkHighAvailabilityServices(configuration, new SimpleMeterRegistry())); | |||
There was a problem hiding this comment.
Can you instead pass MeterRegistry as an arg?
| masterConnectRetryCounter = m.getCounter("MasterConnectRetryCount"); | ||
| this.meterRegistry = meterRegistry; | ||
| String groupName = MasterClientWrapper.class.getCanonicalName(); | ||
| masterConnectRetryCounter = meterRegistry.counter(groupName + ".MasterConnectRetryCount"); |
There was a problem hiding this comment.
minor comment that . should be _
| masterConnectRetryCounter = m.getCounter("MasterConnectRetryCount"); | ||
| this.meterRegistry = meterRegistry; | ||
| String groupName = MasterClientWrapper.class.getCanonicalName(); | ||
| masterConnectRetryCounter = meterRegistry.counter(groupName + ".MasterConnectRetryCount"); |
There was a problem hiding this comment.
Suggested change
| masterConnectRetryCounter = meterRegistry.counter(groupName + ".MasterConnectRetryCount"); | |
| masterConnectRetryCounter = meterRegistry.counter(groupName + "_MasterConnectRetryCount"); |
| this.failureCounter = metrics.getCounter("failureCounter"); | ||
| this.workerSentHeartbeats = metrics.getCounter("workerSentHeartbeats"); | ||
| String groupName = "ReportStatusServiceHttpImpl"; | ||
| this.failureCounter = Counter.builder(groupName + ".failureCounter").register(meterRegistry); |
There was a problem hiding this comment.
Suggested change
| this.failureCounter = Counter.builder(groupName + ".failureCounter").register(meterRegistry); | |
| this.failureCounter = meterRegistry.counter(groupName + "_failureCounter"); |
btw, is there a difference between doing
meterRegistry.counter(...) vs
Counter.builder(...).register(meterRegistry)
If not, I prefer we follow the first approach as much as possible.
| this.workerSentHeartbeats = metrics.getCounter("workerSentHeartbeats"); | ||
| String groupName = "ReportStatusServiceHttpImpl"; | ||
| this.failureCounter = Counter.builder(groupName + ".failureCounter").register(meterRegistry); | ||
| this.workerSentHeartbeats = Counter.builder(groupName + ".workerSentHeartbeats").register(meterRegistry); |
There was a problem hiding this comment.
Suggested change
| this.workerSentHeartbeats = Counter.builder(groupName + ".workerSentHeartbeats").register(meterRegistry); | |
| this.workerSentHeartbeats = Counter.builder(groupName + "_workerSentHeartbeats").register(meterRegistry); |
| this.jobRouteHandler = jobRouteHandler; | ||
| MasterConfiguration config = ConfigurationProvider.getConfig(); | ||
| this.cache = createCache(actorSystem, config.getApiCacheMinSize(), config.getApiCacheMaxSize(), | ||
| config.getApiCacheTtlMilliseconds()); | ||
| this.jobListGET = meterRegistry.counter("V0JobRoute" + "_" +"jobListGET"); |
There was a problem hiding this comment.
Suggested change
| this.jobListGET = meterRegistry.counter("V0JobRoute" + "_" +"jobListGET"); | |
| this.jobListGET = meterRegistry.counter("V0JobRoute_jobListGET"); |
can you apply similar change to following lines as well? thx!
| } | ||
|
|
||
| private Gauge addGauge(String meterName,int value) { | ||
| Gauge gauge = Gauge.builder("JobClusterActor_legacyGaugeCallbackMetricGroup_"+meterName, () -> 1.0 * value) |
There was a problem hiding this comment.
you don't need legacyGaugeCallbackMetricGroup_ infix here because the GaugeCallback used here is io.mantisrx.common.metrics.spectator.GaugeCallback which doesn't use this string.
Also, the value being passed in is incorrect. I believe you want to call addGauge with the lambda and pass it as-is to Gauge.builder.
For eg:
this.acceptedJobsGauge = addGauge("acceptedJobsGauge", () -> 1.0 * this.jobManager.acceptedJobsCount());
and define addGauge as follows:
private Gauge addGauge(String meterName, Supplier<Number> gaugeFun) {
return Gauge.builder("JobClusterActor_" + meterName, gaugeFunc).tags("jobCluster", name).register(meterRegistry);
}
| public void incrementCounter(final String metric, final Iterable<Tag> tags) { | ||
| registry.counter(new MetricId(METRIC_GROUP_ID, metric, tags).getSpectatorId(registry)).increment(); | ||
| public void incrementCounter(final String metric, final Tags tags) { | ||
| meterRegistry.counter(METRIC_GROUP_ID + "_" + metric, tags).increment(); |
| @@ -75,13 +72,13 @@ public void recordMisses(int count) { | |||
| @Override | |||
| public void recordLoadSuccess(long loadTime) { | |||
| loadSuccessCount.increment(); | |||
| totalLoadTime.set(TimeUnit.MILLISECONDS.convert(loadTime, TimeUnit.NANOSECONDS)); | |||
| totalLoadTime.record(Duration.ofDays(TimeUnit.MILLISECONDS.convert(loadTime, TimeUnit.NANOSECONDS))); | |||
There was a problem hiding this comment.
why use Duration.ofDays here?
There was a problem hiding this comment.
Oh! Actually, I was supposed to use Duration.ofNanos here.
| this.noWorkersFoundCounter = m.getCounter("noWorkersFound"); | ||
| this.workersFoundCounter = m.getCounter("workersFound"); | ||
| this.meterRegistry = meterRegistry; | ||
| this.noWorkersFoundCounter = meterRegistry.counter("Storage_noWorkersFound"); |
There was a problem hiding this comment.
minor comment: pl keep storage in lower-case to be consistent with original metrics.
It'd be storage instead of Storage
7ue5wu
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
7ue5wu
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
7ue5wu
had a problem deploying
to
Integrate Pull Request
GitHub Actions
Failure
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Explain context and other details for this pull request.
Checklist
./gradlew buildcompiles code correctly./gradlew testpasses all tests