[jira] [Comment Edited] (IGNITE-11927) [IEP-35] Add ability to enable\disable subset of metrics
[
https://issues.apache.org/jira/browse/IGNITE-11927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16898076#comment-16898076
]
Andrey Gura edited comment on IGNITE-11927 at 8/1/19 1:13 PM:
--
[~NIzhikov] Not certainly in that way. WE don't need NO_OP_METRIC concept
because we don't disable metric itself, we disable whole set of metrics that is
represented by metric registry. So metric registry also should be removed.
Moreover, holder itself can contain logic related with change if metric state.
It should look like:
{code:java}
class MetricHolder {
boolean enabled;
AtomicLongMetric m;
public MetricHolder(GirdKernalContext ctx) {
ctx.monitoring.onDisable("metrics", () -> {
ctx.metric().remove("registryName");
m = null;
}
);
ctx.monitoring.onEnable("metrics", r -> {
MetricRegistry r = new MetricRegistry("registryName");
m = r.longMetric("metric");
ctx.metric().add("registryName");
}
);
}
public long m() {
assert enabled;
return m;
}
public void changeState() {
if (enabled)
m().increment();
}
}
class SomeProcessor {
public SomeProcesor() {
metricHolder = new MetricHolder(ctx);
}
}
{code}
was (Author: agura):
[~NIzhikov] Not certainly in that way. WE don't need NO_OP_METRIC concept
because we don't disable metric itself, we disable whole set of metrics that is
represented by metric registry. So metric registry also should be removed.
Moreover, holder itself can contain logic related with change if metric state.
It should look like:
{code:java}
class MetricHolder {
boolean enabled;
AtomicLongMetric m;
public MetricHolder(GirdKernalContext ctx) {
ctx.monitoring.onDisable("metrics", () -> {
ctx.metric().remove("registryName");
m = null;
}
);
ctx.monitoring.onEnable("metrics", r -> {
MetricRegistry r = new MetricRegistry("registryName");
m = r.longMetric("metric");
ctx.metric().add("registryName");
}
);
}
public long m() {
assert enabled;
return m;
}
public void changeState() {
if (enabled)
m().increment();
}
}
class SomeProcessor {
public SomeProcesor() {
metricHolder = new MetricHolder(ctx);
}
}
{java}
> [IEP-35] Add ability to enable\disable subset of metrics
>
>
> Key: IGNITE-11927
> URL: https://issues.apache.org/jira/browse/IGNITE-11927
> Project: Ignite
> Issue Type: Improvement
>Reporter: Nikolay Izhikov
>Assignee: Nikolay Izhikov
>Priority: Major
> Labels: IEP-35
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Ignite should be able to:
> * Enable or disable an arbitrary subset of the metrics. User should be able
> to do it in runtime.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (IGNITE-11927) [IEP-35] Add ability to enable\disable subset of metrics
[ https://issues.apache.org/jira/browse/IGNITE-11927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16887990#comment-16887990 ] Andrey Gura edited comment on IGNITE-11927 at 7/18/19 2:07 PM: --- [~NIzhikov] > Can you, please, make a simple, pseudo-code example of your idea? Caches, for example. Not pseudo-code, but list of action items. On cache start or metrics enabling on cache: - create metrics holder object. - create MetricRegistry instance. - register metrics in MetricRegistry. - add MetricsRegistry in GridMetricManager. On cache stop or metrics disabling for chache: - remove MetricRegistry from GridMetricManager. - assign null to metrics holder object reference. > 1. If we have 5000 caches, Ignite structures already huge. Why do you think > metrics bring a huge impact on GC? Why we should ignore this impact if we can just avoid it without much effort? > 2. All AtomicLong fields are created in previous versions of > CacheMetricsImpl. MetricRegistry is the only addition we made with the new > framework. You are right. But this addition lead to some changes in design. It's good time to improve implementation. Also, I think it would be better design if MetricRegistry will be immutable. It will lead to more clear code structure and behavior. > Do we have some benchmarks or other descriptions of this issue? No, we don't. But obviously all this objects in heap are not free. was (Author: agura): [~NIzhikov] > Can you, please, make a simple, pseudo-code example of your idea? Caches, for example. Not pseudo-code, but list of action items. On cache start or metrics enabling on cache: - create metrics holder object. - create MetricRegistry instance. - register metrics in MetricRegistry. - add MetricsRegistry in GridMetricManager. On cache stop or metrics disabling for chache: - remove MetricRegistry from GridMetricManager. - assign null to metrics holder object reference. > 1. If we have 5000 caches, Ignite structures already huge. Why do you think > metrics bring a huge impact on GC? Why we should ignore this impact if we can just avoid it without much effort? > 2. All AtomicLong fields are created in previous versions of > CacheMetricsImpl. MetricRegistry is the only addition we made with the new > framework. You are right. But this addition lead to some changes in design. It's good time to improve implementation. > Do we have some benchmarks or other descriptions of this issue? No, we don't. But obviously all this objects in heap are not free. > [IEP-35] Add ability to enable\disable subset of metrics > > > Key: IGNITE-11927 > URL: https://issues.apache.org/jira/browse/IGNITE-11927 > Project: Ignite > Issue Type: Improvement >Reporter: Nikolay Izhikov >Assignee: Nikolay Izhikov >Priority: Major > Labels: IEP-35 > Time Spent: 10m > Remaining Estimate: 0h > > Ignite should be able to: > * Enable or disable an arbitrary subset of the metrics. User should be able > to do it in runtime. -- This message was sent by Atlassian JIRA (v7.6.14#76016)
[jira] [Comment Edited] (IGNITE-11927) [IEP-35] Add ability to enable\disable subset of metrics
[
https://issues.apache.org/jira/browse/IGNITE-11927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16887958#comment-16887958
]
Nikolay Izhikov edited comment on IGNITE-11927 at 7/18/19 1:01 PM:
---
[~agura]
Sorry, I still don't understand you.
Can you, please, make a simple, pseudo-code example of your idea?
> Motivation: reducing memory consuming and GC pressure. There are users with
> big amount of caches (I saw cases with 5000 caches in 200 cache groups).
We can have NoOp implementation of MetricRegistries disabled in the config
file. And this registries never can be enabled.
But this motivation looks very odd for me:
1. If we have 5000 caches, Ignite structures already huge. Why do you think
metrics bring a huge impact on GC?
2. All {{AtomicLong}} fields are created in previous versions of
CacheMetricsImpl. MetricRegistry is the only addition we made with the new
framework.
Do we have some benchmarks or other descriptions of this issue?
was (Author: nizhikov):
[~agura]
Sorry, I still don't understand you.
Can you, please, make a simple, pseudo-code example of your idea?
> Motivation: reducing memory consuming and GC pressure. There are users with
> big amount of caches (I saw cases with 5000 caches in 200 cache groups).
We can have NoOp implementation of MetricRegistries disabled in the config
file. And this registries never can be enabled.
But this motivation looks very odd for me:
1. If we have 5000 caches, Ignite structures already huge. Why do you think
metrics bring a huge impact on GC?
2. All {AtomicLong} fields are created in previous versions of
CacheMetricsImpl. MetricRegistry is the only addition we made with the new
framework.
Do we have some benchmarks or other descriptions of this issue?
> [IEP-35] Add ability to enable\disable subset of metrics
>
>
> Key: IGNITE-11927
> URL: https://issues.apache.org/jira/browse/IGNITE-11927
> Project: Ignite
> Issue Type: Improvement
>Reporter: Nikolay Izhikov
>Assignee: Nikolay Izhikov
>Priority: Major
> Labels: IEP-35
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Ignite should be able to:
> * Enable or disable an arbitrary subset of the metrics. User should be able
> to do it in runtime.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
[jira] [Comment Edited] (IGNITE-11927) [IEP-35] Add ability to enable\disable subset of metrics
[
https://issues.apache.org/jira/browse/IGNITE-11927?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16886762#comment-16886762
]
Nikolay Izhikov edited comment on IGNITE-11927 at 7/17/19 8:28 AM:
---
Hello, Andrey.
> 2. We enable or disable metrics for one source of metrics. So we can just
> remove MetricRegistry from GridMetricManager when metrics disabled.
> 3. There is no need for disabled flag in MetricRegistry. As we discused
> early, when metric disabled they don't consume any memory. MetricsRegistry
> should be collected by GC. After enabling it can be created and registered
> into GridMetricManager again.
I don't understand your proposal.
Metric instances are stored as a class field in the places where they updated.
You can take a {{GridJobProcessor}} or {{CacheMetricImpl}} as examples.
How and why we should clear these variables on disabling?
Can you provide simple pseudo code for disable\enable processing.
> 4. Please, separate buckets configuration and metrics enabling/disabling into
> two different tickets.
OK
> 5. Please, create review in Upsource.
https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1061
> 6. Please, run TC and take a look to results before code review.
https://ci.ignite.apache.org/viewQueued.html?itemId=4343158
was (Author: nizhikov):
Hello, Andrey.
> 2. We enable or disable metrics for one source of metrics. So we can just
> remove MetricRegistry from GridMetricManager when metrics disabled.
> 3. There is no need for disabled flag in MetricRegistry. As we discused
> early, when metric disabled they don't consume any memory. MetricsRegistry
> should be collected by GC. After enabling it can be created and registered
> into GridMetricManager again.
I don't understand your proposal.
Metric instances are stored as a class field in the places where they updated.
You can take a {{GridJobProcessor}} or {{CacheMetricImpl}} as examples.
How and why we should clear these variables on disabling?
Can you provide simple pseudo code for disable\enable processing.
> 4. Please, separate buckets configuration and metrics enabling/disabling into
> two different tickets.
OK
> 5. Please, create review in Upsource.
https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1061
> 6. Please, run TC and take a look to results before code review.
In progress: https://ci.ignite.apache.org/viewLog.html?buildId=4343042;
> [IEP-35] Add ability to enable\disable subset of metrics
>
>
> Key: IGNITE-11927
> URL: https://issues.apache.org/jira/browse/IGNITE-11927
> Project: Ignite
> Issue Type: Improvement
>Reporter: Nikolay Izhikov
>Assignee: Nikolay Izhikov
>Priority: Major
> Labels: IEP-35
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Ignite should be able to:
> * Enable or disable an arbitrary subset of the metrics. User should be able
> to do it in runtime.
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
