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

Dynamic metrics discovery #15650

Merged

Conversation

@blazember
Copy link
Contributor

blazember commented Sep 30, 2019

This change adds dynamic metric discovery along with a few related
changes.

In order to introduce dynamic metrics, the change introduces
a distinction between static and dynamic metrics. The static metrics are
the ones that are added once and never removed during the lifetime of
the instance. These are typically JVM/OS/HZ service stats. The dynamic
stats are the ones that are discovered dynamically in every collection
cycle. The dynamic metrics are mostly data structure metrics and
networking related ones. It would have been possible to get rid of the
static metrics entirely, but collecting metrics dynamically comes with
allocation while static metrics are created once, tenure and stay there
forever without impacting GC. The preliminary measurements showed that
the static metrics consume <20K heap.

Previously to this change, consistent metric naming was introduced. It
converted the legacy metricprefix[metricid].metricname to
[id=metricid,metric=metricprefix.metricname] format. While it has
advantages, it made creating gauges more difficult and error-prone,
since any tag can be added to a metric in any order, and the whole
metric name is needed for creating the gauge. Here the old format was
more useful, so it is kept for gauges. This is done by using the old
format as the key in the MetricsRegistry's static metrics map, while the
new format is added as the name in the ProbeInstances. The metrics
collection hence uses the new format.

The gauges can be created only from static metrics cached in
MetricsRegistry. It would be possible to implement a mechanism that
updates gauges created from dynamic metrics in every collection cycle,
but currently, there are no gauges created from the metrics made dynamic.
It can be added later if needed.

The static vs dynamic distinction has made obvious by changing the
necessary methods, interface and class names.

ProbeBuilder is also renamed to MetricTagger.

EE PR: hazelcast/hazelcast-enterprise#3197

@blazember blazember added this to the 4.0 milestone Sep 30, 2019
@blazember blazember self-assigned this Sep 30, 2019
@blazember blazember force-pushed the blazember:4.0/mc-metrics/dynamic-metrics branch 6 times, most recently from 2be608f to 046ceeb Oct 1, 2019
@blazember blazember marked this pull request as ready for review Oct 3, 2019
@blazember blazember requested a review from hazelcast/clients as a code owner Oct 3, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 3, 2019

run-lab-run

metricsRegistry.collectMetrics(clientExtension);
metricsRegistry.collectMetrics(executionService);
metricsRegistry.registerStaticMetrics(clientExtension.getMemoryStats(), "memory");
metricsRegistry.provideMetrics(clientExtension);

This comment has been minimized.

Copy link
@pveentjer

pveentjer Oct 3, 2019

Member

I can't say I'm terribly excited about the name 'provide metrics'.

The caller will not provide metrics; it will collect the metrics that were provided.

This comment has been minimized.

Copy link
@pveentjer

pveentjer Oct 3, 2019

Member

Ah... this is the registration of metrics; not the collection of metrics.

Perhaps we should just call it something with register? I got confused.. so probably an indication of me either being stupid or the API not being that clear (or a combination of these 2).

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

It's named provideMetrics since we call methods on StaticMetricsProvider instances that will register their metrics. There is an indirection. Also since this change metrics collection is the term used for iterating through the known and discovered metrics and collect (publish, render etc) them. I wanted to avoid the name clash with that too.

@petrpleshachkov petrpleshachkov self-requested a review Oct 3, 2019
* Returns the id for the metric. Used in {@link MetricsRegistryImpl}
* as the key of the metric built with this MetricTagger.
*/
String metricId();

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

We already have metricName? Introducing one more "id" we should at least explain why the existing name could not be used.

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

I made an attempt to improve the javadoc and reason about it.

this.id = null;
this.metricName = null;
Comment on lines +47 to +48

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

Shall we initialize fields with default values?

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

No default values for these, and there are cases where not filled at all.

MetricTaggerImpl tagger = withTag("unit", unit.name().toLowerCase())
.withMetricTag(metricName);
metricsRegistry.registerInternal(source, tagger, level, probe);
// metricsRegistry.registerInternal(source, metricName, name, level, probe);

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

Leftover?

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

Thanks, removed.

collectLong(probeInstance.source, probeInstance.name, (LongProbeFunction) function);
} else if (function instanceof DoubleProbeFunction) {
collectDouble(probeInstance.source, probeInstance.name, (DoubleProbeFunction) function);
}

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

Is any other type of ProbeFunction expected?

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

No. Added an else block to throw, just in case we add more types.

thread.start();
}
this.outputThreads = outThreads;

startIOBalancer();

metricsRegistry.registerDynamicMetricsProvider(this);

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

Shall we register metrics before we start thread?

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

Not necessarily. Here we just register the root of the metrics that will be dynamically and periodically collected later on. I improved provideDynamicMetrics in this class with a null-check for ioBalancer to be on the safe side.

@@ -256,20 +255,19 @@ public void scheduleDeferred(Runnable task, long delay, TimeUnit unit) {
}

private void startAcceptor() {
if (acceptor != null) {
logger.warning("TcpIpAcceptor is already running! Shutting down old acceptor...");
if (acceptorRef.get() != null) {

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

The startAcceptor() is racy if called concurrently. Not a problem of current PR though.

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

It's called from a synchronized method only, so currently it's not possible to call it concurrently.

}

private void shutdownAcceptor() {
TcpIpAcceptor acceptor = acceptorRef.get();

This comment has been minimized.

Copy link
@petrpleshachkov

petrpleshachkov Oct 3, 2019

Contributor

The code is racy if multiple threads try to call shutdownAcceptor

This comment has been minimized.

Copy link
@blazember

blazember Oct 3, 2019

Author Contributor

It's called from a synchronized method only, so currently it's not possible to call it concurrently.

Copy link
Member

asimarslan left a comment

client only

This change adds dynamic metric discovery along with a few related
changes.

In order to introduce dynamic metrics, the change introduces
a distinction between static and dynamic metrics. The static metrics are
the ones that are added once and never removed during the lifetime of
the instance. These are typically JVM/OS/HZ service stats. The dynamic
stats are the ones that are discovered dynamically in every collection
cycle. The dynamic metrics are mostly data structure metrics and
networking related ones. It would have been possible to get rid of the
static metrics entirely, but collecting metrics dynamically comes with
allocation while static metrics are created once, tenure and stay there
forever without impacting GC. The preliminary measurements showed that
the static metrics consume <20K heap.

Previously to this change, consistent metric naming was introduced. It
converted the legacy metricprefix[metricid].metricname to
[id=metricid,metric=metricprefix.metricname] format. While it has
advantages, it made creating gauges more difficult and error-prone,
since any tag can be added to a metric in any order, and the whole
metric name is needed for creating the gauge. Here the old format was
more useful, so it is kept for gauges. This is done by using the old
format as the key in the MetricsRegistry's static metrics map, while the
new format is added as the name in the ProbeInstances. The metrics
collection hence uses the new format.

The gauges can be created only from static metrics cached in
MetricsRegistry. It would be possible to implement a mechanism that
updates gauges created from dynamic metrics in every collection cycle,
but currently, there are no gauges created from the metrics made
dynamic.
It can be added later if needed.

The static vs dynamic distinction has made obvious by changing the
necessary methods, interface and class names.

ProbeBuilder is also renamed to MetricTagger.
@blazember blazember force-pushed the blazember:4.0/mc-metrics/dynamic-metrics branch from 5241cfe to d7c7263 Oct 3, 2019
@blazember blazember merged commit 919ea83 into hazelcast:master Oct 3, 2019
1 check passed
1 check passed
default Test PASSed.
Details
@blazember blazember deleted the blazember:4.0/mc-metrics/dynamic-metrics branch Oct 3, 2019
@blazember

This comment has been minimized.

Copy link
Contributor Author

blazember commented Oct 3, 2019

Thank you for the review guys!

@blazember blazember restored the blazember:4.0/mc-metrics/dynamic-metrics branch Oct 4, 2019
@hazelcast hazelcast deleted a comment from blazember Oct 4, 2019
@hazelcast hazelcast deleted a comment from blazember Oct 4, 2019
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.