Nikolay,
>> 1. There is no unified approach to adding metrics during development.
> Yes, we have it.
> Just call `MetricRegistry#longMetric` or other method and you got your
> counter.
I talk about code structure, not API.
>> Now the logic is spread throughout the code base and there is now any
>> way to find proper place where particular metric could be added to the
>> registry
> You should obtain your counter in the place where it be updated or exported.
And again, I talk about code structure, not API.
>> Moreover, we can create registry in one place and add some
>> set of metrics to the registry in another place and we have many
>> examples of it.
> What's wrong with this approach?
Bad code organization. Redundant synchronization because there is no
real life cases for changing metric registry content at runtime.
>> Of course, metrics API allows it and it is meaningful drawback from my point
>> view
> I know many cases where specific metrics was not implemented because of
> development complexity.
> From my point of view, we shouldn't complicate process of creation of
> specific metric.
> It will slow down creation of new metric therefore slow down product evolving.
My proposal target is simplification of metrics adding to the code.
There is no any complexity.
>> 2. Introduce MetricRegistryBuilder that returns immutable instance of
>> MetricRegistry
> We can extend `AttibuteWalkerGenerator` to generate all required boilerplace
> code (for enabling/disabling metrics, etc.)
I think it is redundant complexity. Code generation is is a
controversial approach. Metrics and system vies is not a case for code
generation. IMHO.
> I want to bold my statement: We should keep metric creation as simple as we
> can.
Please, read my proposal again. Main point is code structure changing
in order to simplify adding new metrics for developer.
> Developer of specific metric shouldn't change dozens of static interfaces,
> implementations, enable/disable methods.
Absolutely agree. But metric sources should have unified life-cycle methods.
> It should only update metric in the specific places in core code.
It is not related with my proposal and will true after implementation.
On Fri, Sep 20, 2019 at 1:03 PM Nikolay Izhikov <nizhi...@apache.org> wrote:
>
> Hello, Andrey.
>
> Thanks for starting this discussion.
>
> > 1. There is no unified approach to adding metrics during development.
>
> Yes, we have it.
> Just call `MetricRegistry#longMetric` or other method and you got your
> counter.
>
> > Now the logic is spread throughout the code base and there is now any
> > way to find proper place where particular metric could be added to the
> > registry
>
> You should obtain your counter in the place where it be updated or exported.
> If you update the same counter in several place, it seems to be as a bad code
> design which should be investigated.
>
> I treat this as a feature that adds flexibility to the Ignite code.
>
> > Moreover, we can create registry in one place and add some
> > set of metrics to the registry in another place and we have many
> > examples of it.
>
> What's wrong with this approach?
> I treat this as a feature, also.
>
> > Of course, metrics API allows it and it is meaningful drawback from my
> > point view
>
> The goal of Metric API was simplifying of metric creation.
> Because, in the past years Ignite doesn't have a way to create meaningfull
> metric.
> I know many cases where specific metrics was not implemented because of
> development complexity.
>
> From my point of view, we shouldn't complicate process of creation of
> specific metric.
> It will slow down creation of new metric therefore slow down product evolving.
>
> > 2. Introduce MetricRegistryBuilder that returns immutable instance of
> > MetricRegistry
>
> If we want immutable MetricRegistry we can create simple POJO objects with
> metric getters as follows
>
> public class CacheMetric {
> public LongMetric putsCount() { ... }
> public LongMetric getsCount() { ... }
> }
>
> and export it using `AttributeWalker` approach I implement in System View
> contribution.
> There is no need for a HashMap unified solution.
>
> We can extend `AttibuteWalkerGenerator` to generate all required boilerplace
> code (for enabling/disabling metrics, etc.)
>
> I want to bold my statement: We should keep metric creation as simple as we
> can.
> Developer of specific metric shouldn't change dozens of static interfaces,
> implementations, enable/disable methods.
> It should only update metric in the specific places in core code.
>
>
> В Пн, 26/08/2019 в 20:36 +0300, Andrey Gura пишет:
> > Hi, Igniters!
> >
> > I'm working on some metrics related aspects and it seems that we have
> > missed some points regarding the metrics management. There are at
> > least two major problems from my point of view.
> >
> > Problem definition.
> > ---------------------------
> >
> > 1. There is no unified approach to adding metrics during development.
> >
> > It would be grate to have one common place where developer should add
> > new metrics or can find answer for question "What kind of metrics do
> > we have for smth?". I talk only about Ignite internal metrics, not
> > user's metrics.
> >
> > Now the logic is spread throughout the code base and there is now any
> > way to find proper place where particular metric could be added to the
> > registry. Moreover, we can create registry in one place and add some
> > set of metrics to the registry in another place and we have many
> > examples of it.
> >
> > Of course, metrics API allows it and it is meaningful drawback from my
> > point view. MetricRegistry is mutable and could be modified at any
> > place of the code and any time of program execution. It allows
> > developer follow the simplest way which is usually wrong. Also
> > GridMetricManager.registry() method has two responsibilities: it
> > creates and lookups registries. When I see registry() method call in
> > the code I can't assume what exactly intention had developer (is it
> > first creation of registry or modification of existing registry).
> >
> > 2. There is no unified approach to enabling/disabling metrics.
> >
> > Here I mean both problems: runtime enabling/disabling and code design
> > that should allow do it in proper way. There is JIRA issue [1] about
> > it.
> >
> > Now some metrics can be enabled/disabled in static configuration and
> > at runtime (cache metrics for example) and other don't have such
> > functionality. It should be unified. Any metrics set (eq.
> > MetricRegistry in current implementation) should have one unified
> > approach for enabling/disabling. Also we should provide infrastructure
> > for it as part of metrics framework.
> >
> >
> > Proposed solution.
> > ---------------------------
> >
> > 1. Introduce new interface, e.g. MetriсSource that is responsible for
> > metrics life cycle for one metrics source (e.g. cache metrics, system
> > metrics, etc.) Each component with metrics should register own
> > implementation of this interface in metrics manager
> > (GridMetricManager).
> >
> > Interface declares the following methods:
> >
> > - String name() - returns sources name that will be available in
> > special MBean responsible for access for metrics manager.
> >
> > - MetricRegistry enable() - returns MetricRegistry instance with
> > allready registered metrics. Metric registry has the same name as name
> > returned by MetricSource.name() method. This method can be invoked on
> > node start (if metrics are statically enabled) or as result of metric
> > enabling via MBean. As result all instance of MetricSource will have
> > all metric instances initialized and MetricRegistry must be added to
> > the registries map by GridMetricManager.
> >
> > - void disable() - Should be invoked after removing corresponding
> > MetricRegistry from GridMetricManager.registries. The responsibility
> > is destroying of all metric instances (just assign null). This method
> > can be invoked during stopping component (e.g. cache stop, index
> > removing) or as result of metric disabling via MBean.
> >
> > - boolean enabled() - Actual state for current implemntation.
> >
> > NOTE 1: Adding/removing metric registries in GridMetricManager should
> > be performed with copy-on-write semantic in order to avoid possible
> > data races in related threads (e.g. exporters). It also allows avoid
> > data copy during exporting (just use link to the registries). Also we
> > can avoid using ConcurrentHashMap in such case. But of course
> > additional copies will be created per each registries map
> > modification. But we assume that it isn't frequent operation.
> >
> > NOTE 2: MetricSource interface specific implementation is also
> > responsible for keeping direct references to metric instances and also
> > for functionality related with metric values calculation (including
> > checking whether metrics are enabled or not).
> >
> > NOTE 3: GridMetricManager is responsible for
> > registration/deregistration of MBeans corresponding to each metrics
> > source. GridMetricManager is responsible for proper actions sequence
> > (e.g. we should first create metric registry and after it add it to
> > registries collection).
> >
> > At this point we have:
> >
> > - Way to find out all metrics in the system just navigating throughout
> > all implementations of MetricSource interface.
> > - Unified approach to metric creation.
> > - Unified approach to metric enabling/disabling at runtime.
> >
> > 2. Introduce MetricRegistryBuilder that returns immutable instance of
> > MetricRegistry. It will force developer to write code related with
> > metric creation at exactly one place. It also allows to avoid
> > MetricRegistry modification at runtime. Also we should remove
> > GridMetricRegistry.registry() method and introduce two new methods:
> > register(MetricRegistry) and unregister(String registryName). Each
> > method should assert that no old value for register() case or registry
> > was deleted for unregister case (we have some problems with it now,
> > e.g. for cache with near cache configuration).
> >
> > At this point we have some additional guaranties of proper metrics
> > framework usage. Such design also will allow optimize metric exporters
> > because it can sure that metric registry will not modified at runtime
> > with out removing and conseqent adding registry to the
> > GridMetricManager.registires map.
> >
> > 3. Introduce MetricManagementMBean that returns list of all metric
> > sources with boolean flag that indicates whether metrics are enabled
> > or disabled for this source. List of all sources should be supported
> > by GridMetricManager (so metricSources collection should be added and
> > accessors).
> >
> > This MBean also should provide methods for enabling and disabling metrics.
> >
> > At this point we have interface for enabling/disabling metrics via JMX
> > and basis for implementation other tools (e.g. we can manage metrics
> > via control.sh after additional development).
> >
> > All current metrics related code should be refactored accordingly this
> > proposal and it is most complex and time consuming part.
> >
> > I'm ready to do all this things. Also anybody can implement and contribute
> > this.
> >
> > But first I would like to hear your thoughts and ideas.
> >
> > Thanks!
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-11927
