Skip to content
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

RATIS-549,RATIS-550,RATIS-552,RATIS-553 Metric Framework for Ratis #19

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ankitsinghal
Copy link

commented May 14, 2019

No description provided.

@ankitsinghal ankitsinghal force-pushed the ankitsinghal:master branch from 54519f7 to da398a2 May 14, 2019

@joshelser
Copy link
Member

left a comment

Looks pretty good Ankit! Thanks for the ping.

If memory serves, a bit of the metrics classes are pulled from HBase. It would be nice to be able to differentiate between what is copy-paste and what is new code you wrote. Not sure if that's easy to define.

@szetszwo you might be interested in this. Giving ratis a standalone metrics framework (built on dropwizard metrics inside). Should give lots of flexibility in the future but also not re-invent the wheel to leverage it today.

.collect(Collectors.toList());
if (x.size() == 1) {
currentGroup = x.get(0);
long start = System.currentTimeMillis();

This comment has been minimized.

Copy link
@joshelser

joshelser May 14, 2019

Member

Using System.nanoTime() would prevent the clock from rolling back. I don't recall if there's any meaningful performance impact of nanoTime() over currentTimeMillis() though.

This comment has been minimized.

Copy link
@ankitsinghal
Show resolved Hide resolved ...e/src/test/java/org/apache/ratis/logservice/LogServiceReadWriteBase.java
.build().start());
}

public static MetricRegistry createMetricRegistryForLogWorker(String name) {

This comment has been minimized.

Copy link
@joshelser

joshelser May 14, 2019

Member

I think LogService specific stuff should exist down in ratis-logservice.

In other words, we would want to treat ratis-logservice as a client of ratis-metrics (and maybe just a reference implementation of how you use ratis-metrics).

This comment has been minimized.

Copy link
@ankitsinghal

ankitsinghal May 14, 2019

Author

Will take care in next commit

Show resolved Hide resolved ratis-metrics/pom.xml Outdated
Show resolved Hide resolved ratis-metrics-api/pom.xml Outdated
Show resolved Hide resolved ratis-metrics-api/pom.xml Outdated

static {
MetricRegistries.global().getMetricRegistries().forEach(
registry -> JmxReporter.forRegistry(new DropwizardMetricRegistryAdaptor(registry))

This comment has been minimized.

Copy link
@joshelser

joshelser May 14, 2019

Member

Just targeting metrics reporting via JMX for now?

Thinking about the case where we want to hook up something else. Would be nice for another layer of indirection where we could let the caller tell us how they want their metrics reported (given what we have code to set up for them 😄 )

This comment has been minimized.

Copy link
@ankitsinghal

ankitsinghal May 14, 2019

Author

Yeah, this is just for illustration. Need to check how our adaptor adopts other reporters of dropwizard.

We already have some kind of indirection by using listeners on metrics registry, where user can implement MetricRegistryListener and register them in metric registry to report metrics in their own way.

Show resolved Hide resolved ...cs/src/main/java/org/apache/ratis/metrics/impl/RatisMetricsRegistry.java Outdated

Ankit Singhal added some commits May 14, 2019

@ankitsinghal ankitsinghal force-pushed the ankitsinghal:master branch from 0adaf52 to 5159cd9 May 16, 2019

@joshelser
Copy link
Member

left a comment

Close now! Thanks for the update Ankit.

Just one or two thoughts.

@@ -230,9 +279,18 @@ public StateMachineStorage getStateMachineStorage() {

}

private CompletableFuture<Message> recordTime(Timer timer, Task task) {

This comment has been minimized.

Copy link
@joshelser

joshelser May 17, 2019

Member

Maybe lift this up to some common, abstract StateMachine implementation so we can use it for both the MetadataStateMachine and the LogStateMachine? Or a utility class is fine too.

Show resolved Hide resolved ...e/src/test/java/org/apache/ratis/logservice/LogServiceReadWriteBase.java
* MetricRegistries is collection of MetricRegistry's. MetricsRegistries implementations should do
* ref-counting of MetricRegistry's via create() and remove() methods.
*/
public abstract class MetricRegistries {

This comment has been minimized.

Copy link
@joshelser

joshelser May 17, 2019

Member

cc/ @jnp

What's your take on keeping a ratis-metrics module? You had expressed offline that you thought wrapping dropwizard metrics classes was overkill -- do you think having this "entrypoint" for Ratis to get/access metrics is OK?

I think it's still good (would help us keep some stability).

return metricRegistry.get();
}
RatisMetricRegistry registry = MetricRegistries.global().create(info);
JmxReporter.forRegistry(registry.getMetricRegistry()).build().start();

This comment has been minimized.

Copy link
@joshelser

joshelser May 17, 2019

Member

I think having a "default" set of reporters we report the metrics to is fine (e.g. JMX is pretty safe), but it would be nice to have a public method which we can call that lets the caller provide a list of Reporters they have configured (gives flexibility to folks downstream like HBase, via LogService, and Ozone).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.