Hello, Andrey. > My proposal target is simplification of metrics adding to the code. > There is no any complexity.
That's great.
Looking forward for your PR.
Please, don't hesitate to request my review for it.
В Ср, 25/09/2019 в 13:23 +0300, Andrey Gura пишет:
> 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
signature.asc
Description: This is a digitally signed message part
