Internal classes are exposed in public API

[Alexey Goncharuk]

Igniters, Nikolay,

I was adding a new metric in the scope of the ticket [1] and was surprised
by a few things:
 * DataRegionMetric public interface is deprecated, however, the suggested
replacement class GridMetricsManager is internal and cannot be acquired by
a user. This makes impossible for the user to fix their code to not use
deprecated API
 * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
MetricRegistry is again an internal class. This will cause the user code
compilation to break when MetricRegistry is changed

These things violate the public-private API separation and need to be fixed
prior 2.8 is released. Let's discuss what changes need to be made to the
API or perhaps move incomplete IEP-35 interfaces to the private package and
remove deprecations until the API is ready.

--AG

[Alexey Goncharuk]

The ticket [1] was https://issues.apache.org/jira/browse/IGNITE-12550

чт, 16 янв. 2020 г. в 16:39, Alexey Goncharuk <[hidden email]>:

> Igniters, Nikolay,
>
> I was adding a new metric in the scope of the ticket [1] and was surprised
> by a few things:
>  * DataRegionMetric public interface is deprecated, however, the suggested
> replacement class GridMetricsManager is internal and cannot be acquired by
> a user. This makes impossible for the user to fix their code to not use
> deprecated API
>  * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
> MetricRegistry is again an internal class. This will cause the user code
> compilation to break when MetricRegistry is changed
>
> These things violate the public-private API separation and need to be
> fixed prior 2.8 is released. Let's discuss what changes need to be made to
> the API or perhaps move incomplete IEP-35 interfaces to the private package
> and remove deprecations until the API is ready.
>
> --AG
>

[Nikolay Izhikov-2]

Hello, Alexey.

>  * DataRegionMetric public interface is deprecated, however, the suggested replacement class GridMetricsManager is internal and cannot be acquired by a user. This makes impossible for the user to fix their code to not use deprecated API

May be it’s not clear text here.
My point here, that user should obtain metrics values via configured metric exporters and don’t use *Metric interfaces.

>  * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but MetricRegistry is again an internal class.

This is a real issue.
We should expose MetricRegistry interface to the public API and keep internal implementation.

I will create ticket and fix it, shortly.

> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <[hidden email]> написал(а):
>
> Igniters, Nikolay,
>
> I was adding a new metric in the scope of the ticket [1] and was surprised by a few things:
>  * DataRegionMetric public interface is deprecated, however, the suggested replacement class GridMetricsManager is internal and cannot be acquired by a user. This makes impossible for the user to fix their code to not use deprecated API
>  * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but MetricRegistry is again an internal class. This will cause the user code compilation to break when MetricRegistry is changed
>
> These things violate the public-private API separation and need to be fixed prior 2.8 is released. Let's discuss what changes need to be made to the API or perhaps move incomplete IEP-35 interfaces to the private package and remove deprecations until the API is ready.
>
> --AG

[Alexey Goncharuk]

Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
obtain an instance of the JMX exporter SPI, how should I map the old
'interface + method name' to the new metric name? I think deprecation of
the public API may be a bit harsh in this case.

чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:

> Hello, Alexey.
>
> >  * DataRegionMetric public interface is deprecated, however, the
> suggested replacement class GridMetricsManager is internal and cannot be
> acquired by a user. This makes impossible for the user to fix their code to
> not use deprecated API
>
> May be it’s not clear text here.
> My point here, that user should obtain metrics values via configured
> metric exporters and don’t use *Metric interfaces.
>
> >  * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
> MetricRegistry is again an internal class.
>
> This is a real issue.
> We should expose MetricRegistry interface to the public API and keep
> internal implementation.
>
> I will create ticket and fix it, shortly.
>
>
> > 16 янв. 2020 г., в 16:39, Alexey Goncharuk <[hidden email]>
> написал(а):
> >
> > Igniters, Nikolay,
> >
> > I was adding a new metric in the scope of the ticket [1] and was
> surprised by a few things:
> >  * DataRegionMetric public interface is deprecated, however, the
> suggested replacement class GridMetricsManager is internal and cannot be
> acquired by a user. This makes impossible for the user to fix their code to
> not use deprecated API
> >  * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
> MetricRegistry is again an internal class. This will cause the user code
> compilation to break when MetricRegistry is changed
> >
> > These things violate the public-private API separation and need to be
> fixed prior 2.8 is released. Let's discuss what changes need to be made to
> the API or perhaps move incomplete IEP-35 interfaces to the private package
> and remove deprecations until the API is ready.
> >
> > --AG
>
>

[Nikolay Izhikov-2]

Do you propose to keep these interfaces forever?

> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <[hidden email]> написал(а):
>
> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
> obtain an instance of the JMX exporter SPI, how should I map the old
> 'interface + method name' to the new metric name? I think deprecation of
> the public API may be a bit harsh in this case.
>
> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
>
>> Hello, Alexey.
>>
>>> * DataRegionMetric public interface is deprecated, however, the
>> suggested replacement class GridMetricsManager is internal and cannot be
>> acquired by a user. This makes impossible for the user to fix their code to
>> not use deprecated API
>>
>> May be it’s not clear text here.
>> My point here, that user should obtain metrics values via configured
>> metric exporters and don’t use *Metric interfaces.
>>
>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>> MetricRegistry is again an internal class.
>>
>> This is a real issue.
>> We should expose MetricRegistry interface to the public API and keep
>> internal implementation.
>>
>> I will create ticket and fix it, shortly.
>>
>>
>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <[hidden email]>
>> написал(а):
>>>
>>> Igniters, Nikolay,
>>>
>>> I was adding a new metric in the scope of the ticket [1] and was
>> surprised by a few things:
>>> * DataRegionMetric public interface is deprecated, however, the
>> suggested replacement class GridMetricsManager is internal and cannot be
>> acquired by a user. This makes impossible for the user to fix their code to
>> not use deprecated API
>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>> MetricRegistry is again an internal class. This will cause the user code
>> compilation to break when MetricRegistry is changed
>>>
>>> These things violate the public-private API separation and need to be
>> fixed prior 2.8 is released. Let's discuss what changes need to be made to
>> the API or perhaps move incomplete IEP-35 interfaces to the private package
>> and remove deprecations until the API is ready.
>>>
>>> --AG
>>
>>

[Nikolay Izhikov-2]

Ticket to export MetricRegistry to the public API created - https://issues.apache.org/jira/browse/IGNITE-12552

> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]> написал(а):
>
> Do you propose to keep these interfaces forever?
>
>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <[hidden email]> написал(а):
>>
>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
>> obtain an instance of the JMX exporter SPI, how should I map the old
>> 'interface + method name' to the new metric name? I think deprecation of
>> the public API may be a bit harsh in this case.
>>
>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
>>
>>> Hello, Alexey.
>>>
>>>> * DataRegionMetric public interface is deprecated, however, the
>>> suggested replacement class GridMetricsManager is internal and cannot be
>>> acquired by a user. This makes impossible for the user to fix their code to
>>> not use deprecated API
>>>
>>> May be it’s not clear text here.
>>> My point here, that user should obtain metrics values via configured
>>> metric exporters and don’t use *Metric interfaces.
>>>
>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>>> MetricRegistry is again an internal class.
>>>
>>> This is a real issue.
>>> We should expose MetricRegistry interface to the public API and keep
>>> internal implementation.
>>>
>>> I will create ticket and fix it, shortly.
>>>
>>>
>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <[hidden email]>
>>> написал(а):
>>>>
>>>> Igniters, Nikolay,
>>>>
>>>> I was adding a new metric in the scope of the ticket [1] and was
>>> surprised by a few things:
>>>> * DataRegionMetric public interface is deprecated, however, the
>>> suggested replacement class GridMetricsManager is internal and cannot be
>>> acquired by a user. This makes impossible for the user to fix their code to
>>> not use deprecated API
>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>>> MetricRegistry is again an internal class. This will cause the user code
>>> compilation to break when MetricRegistry is changed
>>>>
>>>> These things violate the public-private API separation and need to be
>>> fixed prior 2.8 is released. Let's discuss what changes need to be made to
>>> the API or perhaps move incomplete IEP-35 interfaces to the private package
>>> and remove deprecations until the API is ready.
>>>>
>>>> --AG
>>>
>>>
>

[Alexey Goncharuk]

I think it would make sense to make something like a new IgniteMonitoring
facade on Ignite interface and add an ability to obtain a metric value
through this facade, not through an exporter. This would be a
straightforward replacement for all old metrics interfaces.

чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:

> Ticket to export MetricRegistry to the public API created -
> https://issues.apache.org/jira/browse/IGNITE-12552
>
> > 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
> написал(а):
> >
> > Do you propose to keep these interfaces forever?
> >
> >> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <[hidden email]>
> написал(а):
> >>
> >> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
> >> obtain an instance of the JMX exporter SPI, how should I map the old
> >> 'interface + method name' to the new metric name? I think deprecation of
> >> the public API may be a bit harsh in this case.
> >>
> >> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
> >>
> >>> Hello, Alexey.
> >>>
> >>>> * DataRegionMetric public interface is deprecated, however, the
> >>> suggested replacement class GridMetricsManager is internal and cannot
> be
> >>> acquired by a user. This makes impossible for the user to fix their
> code to
> >>> not use deprecated API
> >>>
> >>> May be it’s not clear text here.
> >>> My point here, that user should obtain metrics values via configured
> >>> metric exporters and don’t use *Metric interfaces.
> >>>
> >>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
> >>> MetricRegistry is again an internal class.
> >>>
> >>> This is a real issue.
> >>> We should expose MetricRegistry interface to the public API and keep
> >>> internal implementation.
> >>>
> >>> I will create ticket and fix it, shortly.
> >>>
> >>>
> >>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
> [hidden email]>
> >>> написал(а):
> >>>>
> >>>> Igniters, Nikolay,
> >>>>
> >>>> I was adding a new metric in the scope of the ticket [1] and was
> >>> surprised by a few things:
> >>>> * DataRegionMetric public interface is deprecated, however, the
> >>> suggested replacement class GridMetricsManager is internal and cannot
> be
> >>> acquired by a user. This makes impossible for the user to fix their
> code to
> >>> not use deprecated API
> >>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
> >>> MetricRegistry is again an internal class. This will cause the user
> code
> >>> compilation to break when MetricRegistry is changed
> >>>>
> >>>> These things violate the public-private API separation and need to be
> >>> fixed prior 2.8 is released. Let's discuss what changes need to be
> made to
> >>> the API or perhaps move incomplete IEP-35 interfaces to the private
> package
> >>> and remove deprecations until the API is ready.
> >>>>
> >>>> --AG
> >>>
> >>>
> >
>
>

[Nikolay Izhikov-2]

May be we should enable JMX exporter, by default?
This would provide the same value for the user as `IgniteMonitoring` you propose.

> 16 янв. 2020 г., в 20:06, Alexey Goncharuk <[hidden email]> написал(а):
>
> I think it would make sense to make something like a new IgniteMonitoring
> facade on Ignite interface and add an ability to obtain a metric value
> through this facade, not through an exporter. This would be a
> straightforward replacement for all old metrics interfaces.
>
> чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
>
>> Ticket to export MetricRegistry to the public API created -
>> https://issues.apache.org/jira/browse/IGNITE-12552
>>
>>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
>> написал(а):
>>>
>>> Do you propose to keep these interfaces forever?
>>>
>>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <[hidden email]>
>> написал(а):
>>>>
>>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
>>>> obtain an instance of the JMX exporter SPI, how should I map the old
>>>> 'interface + method name' to the new metric name? I think deprecation of
>>>> the public API may be a bit harsh in this case.
>>>>
>>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
>>>>
>>>>> Hello, Alexey.
>>>>>
>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>> suggested replacement class GridMetricsManager is internal and cannot
>> be
>>>>> acquired by a user. This makes impossible for the user to fix their
>> code to
>>>>> not use deprecated API
>>>>>
>>>>> May be it’s not clear text here.
>>>>> My point here, that user should obtain metrics values via configured
>>>>> metric exporters and don’t use *Metric interfaces.
>>>>>
>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>>>>> MetricRegistry is again an internal class.
>>>>>
>>>>> This is a real issue.
>>>>> We should expose MetricRegistry interface to the public API and keep
>>>>> internal implementation.
>>>>>
>>>>> I will create ticket and fix it, shortly.
>>>>>
>>>>>
>>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
>> [hidden email]>
>>>>> написал(а):
>>>>>>
>>>>>> Igniters, Nikolay,
>>>>>>
>>>>>> I was adding a new metric in the scope of the ticket [1] and was
>>>>> surprised by a few things:
>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>> suggested replacement class GridMetricsManager is internal and cannot
>> be
>>>>> acquired by a user. This makes impossible for the user to fix their
>> code to
>>>>> not use deprecated API
>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>, but
>>>>> MetricRegistry is again an internal class. This will cause the user
>> code
>>>>> compilation to break when MetricRegistry is changed
>>>>>>
>>>>>> These things violate the public-private API separation and need to be
>>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
>> made to
>>>>> the API or perhaps move incomplete IEP-35 interfaces to the private
>> package
>>>>> and remove deprecations until the API is ready.
>>>>>>
>>>>>> --AG
>>>>>
>>>>>
>>>
>>
>>

[Alexey Goncharuk]

That's an option, I agree. Yet for me, from the usability point of view, it
would be quite strange - to get an instance of the monitoring interface, I
would need to do
JmxMetricExporterSpi metrics =
(JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
which is too verbose and fragile if someone changes configuration (say,
inserts another SPI to the first position).

чт, 16 янв. 2020 г. в 20:14, Николай Ижиков <[hidden email]>:

> May be we should enable JMX exporter, by default?
> This would provide the same value for the user as `IgniteMonitoring` you
> propose.
>
> > 16 янв. 2020 г., в 20:06, Alexey Goncharuk <[hidden email]>
> написал(а):
> >
> > I think it would make sense to make something like a new IgniteMonitoring
> > facade on Ignite interface and add an ability to obtain a metric value
> > through this facade, not through an exporter. This would be a
> > straightforward replacement for all old metrics interfaces.
> >
> > чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
> >
> >> Ticket to export MetricRegistry to the public API created -
> >> https://issues.apache.org/jira/browse/IGNITE-12552
> >>
> >>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
> >> написал(а):
> >>>
> >>> Do you propose to keep these interfaces forever?
> >>>
> >>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
> [hidden email]>
> >> написал(а):
> >>>>
> >>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to
> >>>> obtain an instance of the JMX exporter SPI, how should I map the old
> >>>> 'interface + method name' to the new metric name? I think deprecation
> of
> >>>> the public API may be a bit harsh in this case.
> >>>>
> >>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
> >>>>
> >>>>> Hello, Alexey.
> >>>>>
> >>>>>> * DataRegionMetric public interface is deprecated, however, the
> >>>>> suggested replacement class GridMetricsManager is internal and cannot
> >> be
> >>>>> acquired by a user. This makes impossible for the user to fix their
> >> code to
> >>>>> not use deprecated API
> >>>>>
> >>>>> May be it’s not clear text here.
> >>>>> My point here, that user should obtain metrics values via configured
> >>>>> metric exporters and don’t use *Metric interfaces.
> >>>>>
> >>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> but
> >>>>> MetricRegistry is again an internal class.
> >>>>>
> >>>>> This is a real issue.
> >>>>> We should expose MetricRegistry interface to the public API and keep
> >>>>> internal implementation.
> >>>>>
> >>>>> I will create ticket and fix it, shortly.
> >>>>>
> >>>>>
> >>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
> >> [hidden email]>
> >>>>> написал(а):
> >>>>>>
> >>>>>> Igniters, Nikolay,
> >>>>>>
> >>>>>> I was adding a new metric in the scope of the ticket [1] and was
> >>>>> surprised by a few things:
> >>>>>> * DataRegionMetric public interface is deprecated, however, the
> >>>>> suggested replacement class GridMetricsManager is internal and cannot
> >> be
> >>>>> acquired by a user. This makes impossible for the user to fix their
> >> code to
> >>>>> not use deprecated API
> >>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> but
> >>>>> MetricRegistry is again an internal class. This will cause the user
> >> code
> >>>>> compilation to break when MetricRegistry is changed
> >>>>>>
> >>>>>> These things violate the public-private API separation and need to
> be
> >>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
> >> made to
> >>>>> the API or perhaps move incomplete IEP-35 interfaces to the private
> >> package
> >>>>> and remove deprecations until the API is ready.
> >>>>>>
> >>>>>> --AG
> >>>>>
> >>>>>
> >>>
> >>
> >>
>
>

[Pavel Tupitsyn]

Agree with Alexey, there should be an obvious public API to retrieve metrics

On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk <[hidden email]>
wrote:

> That's an option, I agree. Yet for me, from the usability point of view, it
> would be quite strange - to get an instance of the monitoring interface, I
> would need to do
> JmxMetricExporterSpi metrics =
> (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
> which is too verbose and fragile if someone changes configuration (say,
> inserts another SPI to the first position).
>
> чт, 16 янв. 2020 г. в 20:14, Николай Ижиков <[hidden email]>:
>
> > May be we should enable JMX exporter, by default?
> > This would provide the same value for the user as `IgniteMonitoring` you
> > propose.
> >
> > > 16 янв. 2020 г., в 20:06, Alexey Goncharuk <[hidden email]
> >
> > написал(а):
> > >
> > > I think it would make sense to make something like a new
> IgniteMonitoring
> > > facade on Ignite interface and add an ability to obtain a metric value
> > > through this facade, not through an exporter. This would be a
> > > straightforward replacement for all old metrics interfaces.
> > >
> > > чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
> > >
> > >> Ticket to export MetricRegistry to the public API created -
> > >> https://issues.apache.org/jira/browse/IGNITE-12552
> > >>
> > >>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
> > >> написал(а):
> > >>>
> > >>> Do you propose to keep these interfaces forever?
> > >>>
> > >>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
> > [hidden email]>
> > >> написал(а):
> > >>>>
> > >>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
> to
> > >>>> obtain an instance of the JMX exporter SPI, how should I map the old
> > >>>> 'interface + method name' to the new metric name? I think
> deprecation
> > of
> > >>>> the public API may be a bit harsh in this case.
> > >>>>
> > >>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
> > >>>>
> > >>>>> Hello, Alexey.
> > >>>>>
> > >>>>>> * DataRegionMetric public interface is deprecated, however, the
> > >>>>> suggested replacement class GridMetricsManager is internal and
> cannot
> > >> be
> > >>>>> acquired by a user. This makes impossible for the user to fix their
> > >> code to
> > >>>>> not use deprecated API
> > >>>>>
> > >>>>> May be it’s not clear text here.
> > >>>>> My point here, that user should obtain metrics values via
> configured
> > >>>>> metric exporters and don’t use *Metric interfaces.
> > >>>>>
> > >>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> > but
> > >>>>> MetricRegistry is again an internal class.
> > >>>>>
> > >>>>> This is a real issue.
> > >>>>> We should expose MetricRegistry interface to the public API and
> keep
> > >>>>> internal implementation.
> > >>>>>
> > >>>>> I will create ticket and fix it, shortly.
> > >>>>>
> > >>>>>
> > >>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
> > >> [hidden email]>
> > >>>>> написал(а):
> > >>>>>>
> > >>>>>> Igniters, Nikolay,
> > >>>>>>
> > >>>>>> I was adding a new metric in the scope of the ticket [1] and was
> > >>>>> surprised by a few things:
> > >>>>>> * DataRegionMetric public interface is deprecated, however, the
> > >>>>> suggested replacement class GridMetricsManager is internal and
> cannot
> > >> be
> > >>>>> acquired by a user. This makes impossible for the user to fix their
> > >> code to
> > >>>>> not use deprecated API
> > >>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> > but
> > >>>>> MetricRegistry is again an internal class. This will cause the user
> > >> code
> > >>>>> compilation to break when MetricRegistry is changed
> > >>>>>>
> > >>>>>> These things violate the public-private API separation and need to
> > be
> > >>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
> > >> made to
> > >>>>> the API or perhaps move incomplete IEP-35 interfaces to the private
> > >> package
> > >>>>> and remove deprecations until the API is ready.
> > >>>>>>
> > >>>>>> --AG
> > >>>>>
> > >>>>>
> > >>>
> > >>
> > >>
> >
> >
>

[Nikolay Izhikov-2]

Hello, Pavel.

> there should be an obvious public API to retrieve metrics

What do you mean by «retrieve»?
I don’t create pure Java API for metric intentionally.
We have MetricExporterSPI for this purpose.
It very simple from my point of view.
What use cases for `Ignite.monitoring()` API exists?

```
public interface MetricExporterSpi extends IgniteSpi {
    public void setMetricRegistry(ReadOnlyMetricRegistry registry);
    public void setExportFilter(Predicate<MetricRegistry> filter);
}
```

```
public interface ReadOnlyMetricRegistry extends Iterable<MetricRegistry> {
    public void addMetricRegistryCreationListener(Consumer<MetricRegistry> lsnr);
    public void addMetricRegistryRemoveListener(Consumer<MetricRegistry> lsnr);
}
```

```
public class MetricRegistry implements Iterable<Metric> {
    public String name();
    @Nullable public <M extends Metric> M findMetric(String name);
}
```

> 16 янв. 2020 г., в 20:32, Pavel Tupitsyn <[hidden email]> написал(а):
>
> Agree with Alexey, there should be an obvious public API to retrieve metrics
>
> On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk <[hidden email]>
> wrote:
>
>> That's an option, I agree. Yet for me, from the usability point of view, it
>> would be quite strange - to get an instance of the monitoring interface, I
>> would need to do
>> JmxMetricExporterSpi metrics =
>> (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
>> which is too verbose and fragile if someone changes configuration (say,
>> inserts another SPI to the first position).
>>
>> чт, 16 янв. 2020 г. в 20:14, Николай Ижиков <[hidden email]>:
>>
>>> May be we should enable JMX exporter, by default?
>>> This would provide the same value for the user as `IgniteMonitoring` you
>>> propose.
>>>
>>>> 16 янв. 2020 г., в 20:06, Alexey Goncharuk <[hidden email]
>>>
>>> написал(а):
>>>>
>>>> I think it would make sense to make something like a new
>> IgniteMonitoring
>>>> facade on Ignite interface and add an ability to obtain a metric value
>>>> through this facade, not through an exporter. This would be a
>>>> straightforward replacement for all old metrics interfaces.
>>>>
>>>> чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
>>>>
>>>>> Ticket to export MetricRegistry to the public API created -
>>>>> https://issues.apache.org/jira/browse/IGNITE-12552
>>>>>
>>>>>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
>>>>> написал(а):
>>>>>>
>>>>>> Do you propose to keep these interfaces forever?
>>>>>>
>>>>>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
>>> [hidden email]>
>>>>> написал(а):
>>>>>>>
>>>>>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
>> to
>>>>>>> obtain an instance of the JMX exporter SPI, how should I map the old
>>>>>>> 'interface + method name' to the new metric name? I think
>> deprecation
>>> of
>>>>>>> the public API may be a bit harsh in this case.
>>>>>>>
>>>>>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
>>>>>>>
>>>>>>>> Hello, Alexey.
>>>>>>>>
>>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>>>>> suggested replacement class GridMetricsManager is internal and
>> cannot
>>>>> be
>>>>>>>> acquired by a user. This makes impossible for the user to fix their
>>>>> code to
>>>>>>>> not use deprecated API
>>>>>>>>
>>>>>>>> May be it’s not clear text here.
>>>>>>>> My point here, that user should obtain metrics values via
>> configured
>>>>>>>> metric exporters and don’t use *Metric interfaces.
>>>>>>>>
>>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
>>> but
>>>>>>>> MetricRegistry is again an internal class.
>>>>>>>>
>>>>>>>> This is a real issue.
>>>>>>>> We should expose MetricRegistry interface to the public API and
>> keep
>>>>>>>> internal implementation.
>>>>>>>>
>>>>>>>> I will create ticket and fix it, shortly.
>>>>>>>>
>>>>>>>>
>>>>>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
>>>>> [hidden email]>
>>>>>>>> написал(а):
>>>>>>>>>
>>>>>>>>> Igniters, Nikolay,
>>>>>>>>>
>>>>>>>>> I was adding a new metric in the scope of the ticket [1] and was
>>>>>>>> surprised by a few things:
>>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>>>>> suggested replacement class GridMetricsManager is internal and
>> cannot
>>>>> be
>>>>>>>> acquired by a user. This makes impossible for the user to fix their
>>>>> code to
>>>>>>>> not use deprecated API
>>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
>>> but
>>>>>>>> MetricRegistry is again an internal class. This will cause the user
>>>>> code
>>>>>>>> compilation to break when MetricRegistry is changed
>>>>>>>>>
>>>>>>>>> These things violate the public-private API separation and need to
>>> be
>>>>>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
>>>>> made to
>>>>>>>> the API or perhaps move incomplete IEP-35 interfaces to the private
>>>>> package
>>>>>>>> and remove deprecations until the API is ready.
>>>>>>>>>
>>>>>>>>> --AG
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>
>>>
>>

[Pavel Tupitsyn]

Николай, can you provide a full example - how do I get a metric value in my
code as an Ignite user?

var ignite = Ignition.ignite();
var totalAllocatedPages = ???;

On Fri, Jan 17, 2020 at 12:47 AM Николай Ижиков <[hidden email]> wrote:

> Hello, Pavel.
>
> > there should be an obvious public API to retrieve metrics
>
> What do you mean by «retrieve»?
> I don’t create pure Java API for metric intentionally.
> We have MetricExporterSPI for this purpose.
> It very simple from my point of view.
> What use cases for `Ignite.monitoring()` API exists?
>
> ```
> public interface MetricExporterSpi extends IgniteSpi {
>     public void setMetricRegistry(ReadOnlyMetricRegistry registry);
>     public void setExportFilter(Predicate<MetricRegistry> filter);
> }
> ```
>
> ```
> public interface ReadOnlyMetricRegistry extends Iterable<MetricRegistry> {
>     public void addMetricRegistryCreationListener(Consumer<MetricRegistry>
> lsnr);
>     public void addMetricRegistryRemoveListener(Consumer<MetricRegistry>
> lsnr);
> }
> ```
>
> ```
> public class MetricRegistry implements Iterable<Metric> {
>     public String name();
>     @Nullable public <M extends Metric> M findMetric(String name);
> }
> ```
>
> > 16 янв. 2020 г., в 20:32, Pavel Tupitsyn <[hidden email]>
> написал(а):
> >
> > Agree with Alexey, there should be an obvious public API to retrieve
> metrics
> >
> > On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk <
> [hidden email]>
> > wrote:
> >
> >> That's an option, I agree. Yet for me, from the usability point of
> view, it
> >> would be quite strange - to get an instance of the monitoring
> interface, I
> >> would need to do
> >> JmxMetricExporterSpi metrics =
> >> (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
> >> which is too verbose and fragile if someone changes configuration (say,
> >> inserts another SPI to the first position).
> >>
> >> чт, 16 янв. 2020 г. в 20:14, Николай Ижиков <[hidden email]>:
> >>
> >>> May be we should enable JMX exporter, by default?
> >>> This would provide the same value for the user as `IgniteMonitoring`
> you
> >>> propose.
> >>>
> >>>> 16 янв. 2020 г., в 20:06, Alexey Goncharuk <
> [hidden email]
> >>>
> >>> написал(а):
> >>>>
> >>>> I think it would make sense to make something like a new
> >> IgniteMonitoring
> >>>> facade on Ignite interface and add an ability to obtain a metric value
> >>>> through this facade, not through an exporter. This would be a
> >>>> straightforward replacement for all old metrics interfaces.
> >>>>
> >>>> чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
> >>>>
> >>>>> Ticket to export MetricRegistry to the public API created -
> >>>>> https://issues.apache.org/jira/browse/IGNITE-12552
> >>>>>
> >>>>>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
> >>>>> написал(а):
> >>>>>>
> >>>>>> Do you propose to keep these interfaces forever?
> >>>>>>
> >>>>>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
> >>> [hidden email]>
> >>>>> написал(а):
> >>>>>>>
> >>>>>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
> >> to
> >>>>>>> obtain an instance of the JMX exporter SPI, how should I map the
> old
> >>>>>>> 'interface + method name' to the new metric name? I think
> >> deprecation
> >>> of
> >>>>>>> the public API may be a bit harsh in this case.
> >>>>>>>
> >>>>>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
> >>>>>>>
> >>>>>>>> Hello, Alexey.
> >>>>>>>>
> >>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
> >>>>>>>> suggested replacement class GridMetricsManager is internal and
> >> cannot
> >>>>> be
> >>>>>>>> acquired by a user. This makes impossible for the user to fix
> their
> >>>>> code to
> >>>>>>>> not use deprecated API
> >>>>>>>>
> >>>>>>>> May be it’s not clear text here.
> >>>>>>>> My point here, that user should obtain metrics values via
> >> configured
> >>>>>>>> metric exporters and don’t use *Metric interfaces.
> >>>>>>>>
> >>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> >>> but
> >>>>>>>> MetricRegistry is again an internal class.
> >>>>>>>>
> >>>>>>>> This is a real issue.
> >>>>>>>> We should expose MetricRegistry interface to the public API and
> >> keep
> >>>>>>>> internal implementation.
> >>>>>>>>
> >>>>>>>> I will create ticket and fix it, shortly.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
> >>>>> [hidden email]>
> >>>>>>>> написал(а):
> >>>>>>>>>
> >>>>>>>>> Igniters, Nikolay,
> >>>>>>>>>
> >>>>>>>>> I was adding a new metric in the scope of the ticket [1] and was
> >>>>>>>> surprised by a few things:
> >>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
> >>>>>>>> suggested replacement class GridMetricsManager is internal and
> >> cannot
> >>>>> be
> >>>>>>>> acquired by a user. This makes impossible for the user to fix
> their
> >>>>> code to
> >>>>>>>> not use deprecated API
> >>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
> >>> but
> >>>>>>>> MetricRegistry is again an internal class. This will cause the
> user
> >>>>> code
> >>>>>>>> compilation to break when MetricRegistry is changed
> >>>>>>>>>
> >>>>>>>>> These things violate the public-private API separation and need
> to
> >>> be
> >>>>>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
> >>>>> made to
> >>>>>>>> the API or perhaps move incomplete IEP-35 interfaces to the
> private
> >>>>> package
> >>>>>>>> and remove deprecations until the API is ready.
> >>>>>>>>>
> >>>>>>>>> --AG
> >>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >>
>
>

[Nikolay Izhikov-2]

Pavel,

As I said before - I don’t see any real use-cases for your example.

That’s why

> I don’t create pure Java API for metric intentionally.

What real-world use-case you are keeping in mind?
Why do you want to do it in that way?

The API you’re talking about is very simple and straightforward.
It’s an easy thing to create, but this will open to the user's wrong usage pattern.
From my point of view, to get metrics values one should use MetricExporterSPI.

> 17 янв. 2020 г., в 09:40, Pavel Tupitsyn <[hidden email]> написал(а):
>
> Николай, can you provide a full example - how do I get a metric value in my
> code as an Ignite user?
>
> var ignite = Ignition.ignite();
> var totalAllocatedPages = ???;
>
> On Fri, Jan 17, 2020 at 12:47 AM Николай Ижиков <[hidden email]> wrote:
>
>> Hello, Pavel.
>>
>>> there should be an obvious public API to retrieve metrics
>>
>> What do you mean by «retrieve»?
>> I don’t create pure Java API for metric intentionally.
>> We have MetricExporterSPI for this purpose.
>> It very simple from my point of view.
>> What use cases for `Ignite.monitoring()` API exists?
>>
>> ```
>> public interface MetricExporterSpi extends IgniteSpi {
>>    public void setMetricRegistry(ReadOnlyMetricRegistry registry);
>>    public void setExportFilter(Predicate<MetricRegistry> filter);
>> }
>> ```
>>
>> ```
>> public interface ReadOnlyMetricRegistry extends Iterable<MetricRegistry> {
>>    public void addMetricRegistryCreationListener(Consumer<MetricRegistry>
>> lsnr);
>>    public void addMetricRegistryRemoveListener(Consumer<MetricRegistry>
>> lsnr);
>> }
>> ```
>>
>> ```
>> public class MetricRegistry implements Iterable<Metric> {
>>    public String name();
>>    @Nullable public <M extends Metric> M findMetric(String name);
>> }
>> ```
>>
>>> 16 янв. 2020 г., в 20:32, Pavel Tupitsyn <[hidden email]>
>> написал(а):
>>>
>>> Agree with Alexey, there should be an obvious public API to retrieve
>> metrics
>>>
>>> On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk <
>> [hidden email]>
>>> wrote:
>>>
>>>> That's an option, I agree. Yet for me, from the usability point of
>> view, it
>>>> would be quite strange - to get an instance of the monitoring
>> interface, I
>>>> would need to do
>>>> JmxMetricExporterSpi metrics =
>>>> (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
>>>> which is too verbose and fragile if someone changes configuration (say,
>>>> inserts another SPI to the first position).
>>>>
>>>> чт, 16 янв. 2020 г. в 20:14, Николай Ижиков <[hidden email]>:
>>>>
>>>>> May be we should enable JMX exporter, by default?
>>>>> This would provide the same value for the user as `IgniteMonitoring`
>> you
>>>>> propose.
>>>>>
>>>>>> 16 янв. 2020 г., в 20:06, Alexey Goncharuk <
>> [hidden email]
>>>>>
>>>>> написал(а):
>>>>>>
>>>>>> I think it would make sense to make something like a new
>>>> IgniteMonitoring
>>>>>> facade on Ignite interface and add an ability to obtain a metric value
>>>>>> through this facade, not through an exporter. This would be a
>>>>>> straightforward replacement for all old metrics interfaces.
>>>>>>
>>>>>> чт, 16 янв. 2020 г. в 17:13, Николай Ижиков <[hidden email]>:
>>>>>>
>>>>>>> Ticket to export MetricRegistry to the public API created -
>>>>>>> https://issues.apache.org/jira/browse/IGNITE-12552
>>>>>>>
>>>>>>>> 16 янв. 2020 г., в 16:58, Николай Ижиков <[hidden email]>
>>>>>>> написал(а):
>>>>>>>>
>>>>>>>> Do you propose to keep these interfaces forever?
>>>>>>>>
>>>>>>>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
>>>>> [hidden email]>
>>>>>>> написал(а):
>>>>>>>>>
>>>>>>>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
>>>> to
>>>>>>>>> obtain an instance of the JMX exporter SPI, how should I map the
>> old
>>>>>>>>> 'interface + method name' to the new metric name? I think
>>>> deprecation
>>>>> of
>>>>>>>>> the public API may be a bit harsh in this case.
>>>>>>>>>
>>>>>>>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков <[hidden email]>:
>>>>>>>>>
>>>>>>>>>> Hello, Alexey.
>>>>>>>>>>
>>>>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>>>>>>> suggested replacement class GridMetricsManager is internal and
>>>> cannot
>>>>>>> be
>>>>>>>>>> acquired by a user. This makes impossible for the user to fix
>> their
>>>>>>> code to
>>>>>>>>>> not use deprecated API
>>>>>>>>>>
>>>>>>>>>> May be it’s not clear text here.
>>>>>>>>>> My point here, that user should obtain metrics values via
>>>> configured
>>>>>>>>>> metric exporters and don’t use *Metric interfaces.
>>>>>>>>>>
>>>>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
>>>>> but
>>>>>>>>>> MetricRegistry is again an internal class.
>>>>>>>>>>
>>>>>>>>>> This is a real issue.
>>>>>>>>>> We should expose MetricRegistry interface to the public API and
>>>> keep
>>>>>>>>>> internal implementation.
>>>>>>>>>>
>>>>>>>>>> I will create ticket and fix it, shortly.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
>>>>>>> [hidden email]>
>>>>>>>>>> написал(а):
>>>>>>>>>>>
>>>>>>>>>>> Igniters, Nikolay,
>>>>>>>>>>>
>>>>>>>>>>> I was adding a new metric in the scope of the ticket [1] and was
>>>>>>>>>> surprised by a few things:
>>>>>>>>>>> * DataRegionMetric public interface is deprecated, however, the
>>>>>>>>>> suggested replacement class GridMetricsManager is internal and
>>>> cannot
>>>>>>> be
>>>>>>>>>> acquired by a user. This makes impossible for the user to fix
>> their
>>>>>>> code to
>>>>>>>>>> not use deprecated API
>>>>>>>>>>> * In ReadOnlyMetricsRegistry there is a Consumer<MetricRegistry>,
>>>>> but
>>>>>>>>>> MetricRegistry is again an internal class. This will cause the
>> user
>>>>>>> code
>>>>>>>>>> compilation to break when MetricRegistry is changed
>>>>>>>>>>>
>>>>>>>>>>> These things violate the public-private API separation and need
>> to
>>>>> be
>>>>>>>>>> fixed prior 2.8 is released. Let's discuss what changes need to be
>>>>>>> made to
>>>>>>>>>> the API or perhaps move incomplete IEP-35 interfaces to the
>> private
>>>>>>> package
>>>>>>>>>> and remove deprecations until the API is ready.
>>>>>>>>>>>
>>>>>>>>>>> --AG
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>
>>

[Alexey Goncharuk]

Nikolay,

Why do you think this is a wrong usage pattern? From the top of my head,
here is a few cases of direct metric API usage that I know are currently
being used in production:
 * A custom task execution scheduling service with load balancing based on
utilization metrics readings from Java code
 * Cleanup task trigger based on metrics readings
 * A custom health-check endpoint for an application with an embedded
Ignite node for Kubernetes/Spring Application/etc

[Nikolay Izhikov-2]

Alex.

OK, I may leverage your experience and create pure Java API.
Ticket [1] created.

But, personally, I don’t agree with you.
Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages.
Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users.

You can just see count of the tests we just mute on the TC.

Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3]

[1] https://issues.apache.org/jira/browse/IGNITE-12553
[2] https://github.com/apache/ignite/pull/7269
[3] https://issues.apache.org/jira/browse/IGNITE-12552

> 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]> написал(а):
>
> Nikolay,
>
> Why do you think this is a wrong usage pattern? From the top of my head,
> here is a few cases of direct metric API usage that I know are currently
> being used in production:
> * A custom task execution scheduling service with load balancing based on
> utilization metrics readings from Java code
> * Cleanup task trigger based on metrics readings
> * A custom health-check endpoint for an application with an embedded
> Ignite node for Kubernetes/Spring Application/etc

[Alexey Goncharuk]

Nikolay, thanks! I am not insisting we must implement IGNITE-12553 right
away nor am I saying it's mandatory. I just want to make sure we do not
deprecate existing APIs unless we as a community are certain that they will
be dropped and there is no need for the replacement.

Will take a look at the PR shortly.

пт, 17 янв. 2020 г. в 12:49, Николай Ижиков <[hidden email]>:

> Alex.
>
> OK, I may leverage your experience and create pure Java API.
> Ticket [1] created.
>
> But, personally, I don’t agree with you.
> Ignite has dozens of the API that theoretically have a usage scenario, but
> in real-world have 0 custom implementation and usages.
> Moreover, many APIs that were created with the intentions you mentioned is
> abandoned now and confuses users.
>
> You can just see count of the tests we just mute on the TC.
>
> Can you, please, take a look at the fix regarding puck API issue you
> mentioned in your first letter [2], [3]
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12553
> [2] https://github.com/apache/ignite/pull/7269
> [3] https://issues.apache.org/jira/browse/IGNITE-12552
>
>
> > 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]>
> написал(а):
> >
> > Nikolay,
> >
> > Why do you think this is a wrong usage pattern? From the top of my head,
> > here is a few cases of direct metric API usage that I know are currently
> > being used in production:
> > * A custom task execution scheduling service with load balancing based on
> > utilization metrics readings from Java code
> > * Cleanup task trigger based on metrics readings
> > * A custom health-check endpoint for an application with an embedded
> > Ignite node for Kubernetes/Spring Application/etc
>
>

[agura]

Hi,

The first, I agree with Alexey about deprecation of APIs that are
still supported and don't offer reasonable substitution.

The second, from my point of view, we can't recommend
MetricExporterSpi's because it are still not-production ready. There
are some issues with it and usage of ReadOnlyMetricRegistry interface
just one of them.

The third, moving of MetricRegistry to the public API doesn't solve
the problem because this interface exposes internal Metric interface
implementations. So your PR is incomplete.
Moreover, API of MetricRegistry is inconsistent. E.g. register(name,
supplier, desc) method returns registered metric for some types and
doesn't for other. register(metric) method is inconsistent in sense of
metric naming. ReadOnlyMetricRegistry interface is redundant.
MetricExporterSpi should be revised because it absolutely not
intuitive because it requires ReadOnlyMetricRegistry and it's purpose
is undefined.

One more point. IEP-35 is still not fully implemented. Some things are
not taken into account. Exporters expose metrics if they are disabled.
JMX beans exposes values that don't confirm to best practices [1].
Metrics of type lists are not metric at all. Ubiquitous merics lookup
from hash map instead of usage reference for getting metrics values
(it is just performance issue). Also IGNITE-11927 is not implemented
which also changes interfaces significantly.

Let's just admit that the implementation is immature and must be moved
to the internal packages.

And because we already merged partially implemented IEP to the master
branch we *must move all currently public APIs to the internal API*
while it will not be ready for publication.

And the last but not least. What is happening indicates a immature
development process which must be revised. I don't want discuss it in
this thread but we must not allow merge of change to the master branch
before it will completed, that is we must use feature branches for
full IEP not only for particular tickets. And also we should
reformulate IEP process in order to avoid things like this.

[1] https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html

On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков <[hidden email]> wrote:

>
> Alex.
>
> OK, I may leverage your experience and create pure Java API.
> Ticket [1] created.
>
> But, personally, I don’t agree with you.
> Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages.
> Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users.
>
> You can just see count of the tests we just mute on the TC.
>
> Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3]
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12553
> [2] https://github.com/apache/ignite/pull/7269
> [3] https://issues.apache.org/jira/browse/IGNITE-12552
>
>
> > 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]> написал(а):
> >
> > Nikolay,
> >
> > Why do you think this is a wrong usage pattern? From the top of my head,
> > here is a few cases of direct metric API usage that I know are currently
> > being used in production:
> > * A custom task execution scheduling service with load balancing based on
> > utilization metrics readings from Java code
> > * Cleanup task trigger based on metrics readings
> > * A custom health-check endpoint for an application with an embedded
> > Ignite node for Kubernetes/Spring Application/etc
>

[Nikolay Izhikov-2]

Andrey, thanks for your opinion and your ownest critisism.
I can’t wait for your contribution.

If I’m not missing something, you were one of the active reviewers of the Metric API.

> The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution.

It has - MetricExporterSPI.

> The second, from my point of view, we can't recommend MetricExporterSpi's because it are still not-production ready.

It’s ready.

> The third, moving of MetricRegistry to the public API doesn't solve the problem because this interface exposes internal Metric interface implementations.

Not, its’ not.
Please, see `org.apache.ignite.spi.metric.LongMetric` and other public interface.

> API of MetricRegistry is inconsistent.

MetricRegistry is the internal API.
Feel free to create ticket for an issues with it and I will try to fix it.

> ReadOnlyMetricRegistry interface is redundant.

It’s an interface that exposes internal MetricRegistry  to the exporters.

> Exporters expose metrics if they are disabled.

We don’t have an ability to disable metrics.
And that done, intentionally.
You are working on this issue, aren’t you?
I can fix this issue, by myself.

> Metrics of type lists are not metric at all.

They are created to deal with backward compatibility.

> 17 янв. 2020 г., в 15:09, Andrey Gura <[hidden email]> написал(а):
>
> Hi,
>
> The first, I agree with Alexey about deprecation of APIs that are
> still supported and don't offer reasonable substitution.
>
> The second, from my point of view, we can't recommend
> MetricExporterSpi's because it are still not-production ready. There
> are some issues with it and usage of ReadOnlyMetricRegistry interface
> just one of them.
>
> The third, moving of MetricRegistry to the public API doesn't solve
> the problem because this interface exposes internal Metric interface
> implementations. So your PR is incomplete.
> Moreover, API of MetricRegistry is inconsistent. E.g. register(name,
> supplier, desc) method returns registered metric for some types and
> doesn't for other. register(metric) method is inconsistent in sense of
> metric naming. ReadOnlyMetricRegistry interface is redundant.
> MetricExporterSpi should be revised because it absolutely not
> intuitive because it requires ReadOnlyMetricRegistry and it's purpose
> is undefined.
>
> One more point. IEP-35 is still not fully implemented. Some things are
> not taken into account. Exporters expose metrics if they are disabled.
> JMX beans exposes values that don't confirm to best practices [1].
> Metrics of type lists are not metric at all. Ubiquitous merics lookup
> from hash map instead of usage reference for getting metrics values
> (it is just performance issue). Also IGNITE-11927 is not implemented
> which also changes interfaces significantly.
>
> Let's just admit that the implementation is immature and must be moved
> to the internal packages.
>
> And because we already merged partially implemented IEP to the master
> branch we *must move all currently public APIs to the internal API*
> while it will not be ready for publication.
>
> And the last but not least. What is happening indicates a immature
> development process which must be revised. I don't want discuss it in
> this thread but we must not allow merge of change to the master branch
> before it will completed, that is we must use feature branches for
> full IEP not only for particular tickets. And also we should
> reformulate IEP process in order to avoid things like this.
>
> [1] https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html
>
> On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков <[hidden email]> wrote:
>>
>> Alex.
>>
>> OK, I may leverage your experience and create pure Java API.
>> Ticket [1] created.
>>
>> But, personally, I don’t agree with you.
>> Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages.
>> Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users.
>>
>> You can just see count of the tests we just mute on the TC.
>>
>> Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3]
>>
>> [1] https://issues.apache.org/jira/browse/IGNITE-12553
>> [2] https://github.com/apache/ignite/pull/7269
>> [3] https://issues.apache.org/jira/browse/IGNITE-12552
>>
>>
>>> 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]> написал(а):
>>>
>>> Nikolay,
>>>
>>> Why do you think this is a wrong usage pattern? From the top of my head,
>>> here is a few cases of direct metric API usage that I know are currently
>>> being used in production:
>>> * A custom task execution scheduling service with load balancing based on
>>> utilization metrics readings from Java code
>>> * Cleanup task trigger based on metrics readings
>>> * A custom health-check endpoint for an application with an embedded
>>> Ignite node for Kubernetes/Spring Application/etc
>>

[agura]

> If I’m not missing something, you were one of the active reviewers of the Metric API.

Yes. But if I'm not missing some thing you were major developer of
Metric API :) Shit happens. And it happened.

>> The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution.
> It has - MetricExporterSPI.

There is such concept - backward compatibility. I understand that
deprecation of some interface doesn't break backward compatibility but
it leads to question^ what should I use instead of this. And
MetricExporterSpi is not answer for this question. Because it is brand
new API and it requires rewrite client code.

>> ReadOnlyMetricRegistry interface is redundant.
> It’s an interface that exposes internal MetricRegistry  to the exporters.

No it is not. It's completely artificial thing which allow iterate via
all metric registries. GridMetricManager implements this interface
while it is not metric registry. Form user stand point it is very
strange interface which don't give me any information about it's
purpose and responsibilities.

>> Exporters expose metrics if they are disabled.
> We don’t have an ability to disable metrics.

Actually not. We have statisticsEnabled for caches for example. There
are other entities with such flag.

> And that done, intentionally.

Why do you decided do in such way? Why you ignore existing
functionality? It affects user expectations and experience.

> You are working on this issue, aren’t you?

Yes? I'm working. Unfortunately we are not synchronized in this
context and I should redo all metrics related changes in order to
merge it with my changes. Anyway, my change doesn't solve all problems
(e.g. it doesn't introduce IgniteMonitoring facade).

> I can fix this issue, by myself.

Unfortunately it will be just fix. In my solution it is redesign. Stop
fixing issues, let's do things. It requires deeper changes. My changes
blocks AI 2.8 release because it big enough. So it retargeted on the
next release. And it is one more reason for moving the changes to the
internal packages. And it isn't good news for me because I will go
throughout pan and tiers of merge. But it is right.

> Metrics of type lists are not metric at all.

They are created to deal with backward compatibility.

>> Metrics of type lists are not metric at all.
> They are created to deal with backward compatibility.

Yes, I know. But they should not be exported by MetricExporterSpi
implementations.

On Fri, Jan 17, 2020 at 3:37 PM Николай Ижиков <[hidden email]> wrote:

>
> Andrey, thanks for your opinion and your ownest critisism.
> I can’t wait for your contribution.
>
> If I’m not missing something, you were one of the active reviewers of the Metric API.
>
> > The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution.
>
> It has - MetricExporterSPI.
>
> > The second, from my point of view, we can't recommend MetricExporterSpi's because it are still not-production ready.
>
> It’s ready.
>
> > The third, moving of MetricRegistry to the public API doesn't solve the problem because this interface exposes internal Metric interface implementations.
>
> Not, its’ not.
> Please, see `org.apache.ignite.spi.metric.LongMetric` and other public interface.
>
> > API of MetricRegistry is inconsistent.
>
> MetricRegistry is the internal API.
> Feel free to create ticket for an issues with it and I will try to fix it.
>
> > ReadOnlyMetricRegistry interface is redundant.
>
> It’s an interface that exposes internal MetricRegistry  to the exporters.
>
> > Exporters expose metrics if they are disabled.
>
> We don’t have an ability to disable metrics.
> And that done, intentionally.
> You are working on this issue, aren’t you?
> I can fix this issue, by myself.
>
> > Metrics of type lists are not metric at all.
>
> They are created to deal with backward compatibility.
>
> > 17 янв. 2020 г., в 15:09, Andrey Gura <[hidden email]> написал(а):
> >
> > Hi,
> >
> > The first, I agree with Alexey about deprecation of APIs that are
> > still supported and don't offer reasonable substitution.
> >
> > The second, from my point of view, we can't recommend
> > MetricExporterSpi's because it are still not-production ready. There
> > are some issues with it and usage of ReadOnlyMetricRegistry interface
> > just one of them.
> >
> > The third, moving of MetricRegistry to the public API doesn't solve
> > the problem because this interface exposes internal Metric interface
> > implementations. So your PR is incomplete.
> > Moreover, API of MetricRegistry is inconsistent. E.g. register(name,
> > supplier, desc) method returns registered metric for some types and
> > doesn't for other. register(metric) method is inconsistent in sense of
> > metric naming. ReadOnlyMetricRegistry interface is redundant.
> > MetricExporterSpi should be revised because it absolutely not
> > intuitive because it requires ReadOnlyMetricRegistry and it's purpose
> > is undefined.
> >
> > One more point. IEP-35 is still not fully implemented. Some things are
> > not taken into account. Exporters expose metrics if they are disabled.
> > JMX beans exposes values that don't confirm to best practices [1].
> > Metrics of type lists are not metric at all. Ubiquitous merics lookup
> > from hash map instead of usage reference for getting metrics values
> > (it is just performance issue). Also IGNITE-11927 is not implemented
> > which also changes interfaces significantly.
> >
> > Let's just admit that the implementation is immature and must be moved
> > to the internal packages.
> >
> > And because we already merged partially implemented IEP to the master
> > branch we *must move all currently public APIs to the internal API*
> > while it will not be ready for publication.
> >
> > And the last but not least. What is happening indicates a immature
> > development process which must be revised. I don't want discuss it in
> > this thread but we must not allow merge of change to the master branch
> > before it will completed, that is we must use feature branches for
> > full IEP not only for particular tickets. And also we should
> > reformulate IEP process in order to avoid things like this.
> >
> > [1] https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html
> >
> > On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков <[hidden email]> wrote:
> >>
> >> Alex.
> >>
> >> OK, I may leverage your experience and create pure Java API.
> >> Ticket [1] created.
> >>
> >> But, personally, I don’t agree with you.
> >> Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages.
> >> Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users.
> >>
> >> You can just see count of the tests we just mute on the TC.
> >>
> >> Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3]
> >>
> >> [1] https://issues.apache.org/jira/browse/IGNITE-12553
> >> [2] https://github.com/apache/ignite/pull/7269
> >> [3] https://issues.apache.org/jira/browse/IGNITE-12552
> >>
> >>
> >>> 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]> написал(а):
> >>>
> >>> Nikolay,
> >>>
> >>> Why do you think this is a wrong usage pattern? From the top of my head,
> >>> here is a few cases of direct metric API usage that I know are currently
> >>> being used in production:
> >>> * A custom task execution scheduling service with load balancing based on
> >>> utilization metrics readings from Java code
> >>> * Cleanup task trigger based on metrics readings
> >>> * A custom health-check endpoint for an application with an embedded
> >>> Ignite node for Kubernetes/Spring Application/etc
> >>
>

[Nikolay Izhikov-2]

Andrey.

> Because it is brand new API and it requires rewrite client code.

We doesn’t break backward compatibility.
The message is - this interface would be remove in the next major release.

> ReadOnlyMetricRegistry
> Form user stand point it is very strange interface which don't give me any information about it’s purpose and responsibilities.

Seems, I should explain proposed changes [1] more clear:

Each SPI would have a `ReadOnlyMetricManager` which provides access to collection of `ReadOnlyMetricRegistry`
which has a collection of `Metric`.

So we reflects two-level structure we have in the internal API

GridMetricManager -> Collection[MetricRegistry] -> Collection[Metric]
ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry] -> Collection[Metric]

> Actually not. We have statisticsEnabled for caches for example. There are other entities with such flag.

They still works as expected.

> Why do you decided do in such way?

Because of the scope.
The ability to disable/enable metrics is the matter of the other ticket.

> But they should not be exported by MetricExporterSpi implementations.

Actually, it’s a responsibility of the exporter to decide.
JMX exporter can exports ObjectMetric while OpenCensus exporter can’t.

[1] https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25

> 17 янв. 2020 г., в 16:57, Andrey Gura <[hidden email]> написал(а):
>
>> If I’m not missing something, you were one of the active reviewers of the Metric API.
>
> Yes. But if I'm not missing some thing you were major developer of
> Metric API :) Shit happens. And it happened.
>
>>> The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution.
>> It has - MetricExporterSPI.
>
> There is such concept - backward compatibility. I understand that
> deprecation of some interface doesn't break backward compatibility but
> it leads to question^ what should I use instead of this. And
> MetricExporterSpi is not answer for this question. Because it is brand
> new API and it requires rewrite client code.
>
>>> ReadOnlyMetricRegistry interface is redundant.
>> It’s an interface that exposes internal MetricRegistry  to the exporters.
>
> No it is not. It's completely artificial thing which allow iterate via
> all metric registries. GridMetricManager implements this interface
> while it is not metric registry. Form user stand point it is very
> strange interface which don't give me any information about it's
> purpose and responsibilities.
>
>>> Exporters expose metrics if they are disabled.
>> We don’t have an ability to disable metrics.
>
> Actually not. We have statisticsEnabled for caches for example. There
> are other entities with such flag.
>
>> And that done, intentionally.
>
> Why do you decided do in such way? Why you ignore existing
> functionality? It affects user expectations and experience.
>
>> You are working on this issue, aren’t you?
>
> Yes? I'm working. Unfortunately we are not synchronized in this
> context and I should redo all metrics related changes in order to
> merge it with my changes. Anyway, my change doesn't solve all problems
> (e.g. it doesn't introduce IgniteMonitoring facade).
>
>> I can fix this issue, by myself.
>
> Unfortunately it will be just fix. In my solution it is redesign. Stop
> fixing issues, let's do things. It requires deeper changes. My changes
> blocks AI 2.8 release because it big enough. So it retargeted on the
> next release. And it is one more reason for moving the changes to the
> internal packages. And it isn't good news for me because I will go
> throughout pan and tiers of merge. But it is right.
>
>> Metrics of type lists are not metric at all.
>
> They are created to deal with backward compatibility.
>
>>> Metrics of type lists are not metric at all.
>> They are created to deal with backward compatibility.
>
> Yes, I know. But they should not be exported by MetricExporterSpi
> implementations.
>
> On Fri, Jan 17, 2020 at 3:37 PM Николай Ижиков <[hidden email]> wrote:
>>
>> Andrey, thanks for your opinion and your ownest critisism.
>> I can’t wait for your contribution.
>>
>> If I’m not missing something, you were one of the active reviewers of the Metric API.
>>
>>> The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution.
>>
>> It has - MetricExporterSPI.
>>
>>> The second, from my point of view, we can't recommend MetricExporterSpi's because it are still not-production ready.
>>
>> It’s ready.
>>
>>> The third, moving of MetricRegistry to the public API doesn't solve the problem because this interface exposes internal Metric interface implementations.
>>
>> Not, its’ not.
>> Please, see `org.apache.ignite.spi.metric.LongMetric` and other public interface.
>>
>>> API of MetricRegistry is inconsistent.
>>
>> MetricRegistry is the internal API.
>> Feel free to create ticket for an issues with it and I will try to fix it.
>>
>>> ReadOnlyMetricRegistry interface is redundant.
>>
>> It’s an interface that exposes internal MetricRegistry  to the exporters.
>>
>>> Exporters expose metrics if they are disabled.
>>
>> We don’t have an ability to disable metrics.
>> And that done, intentionally.
>> You are working on this issue, aren’t you?
>> I can fix this issue, by myself.
>>
>>> Metrics of type lists are not metric at all.
>>
>> They are created to deal with backward compatibility.
>>
>>> 17 янв. 2020 г., в 15:09, Andrey Gura <[hidden email]> написал(а):
>>>
>>> Hi,
>>>
>>> The first, I agree with Alexey about deprecation of APIs that are
>>> still supported and don't offer reasonable substitution.
>>>
>>> The second, from my point of view, we can't recommend
>>> MetricExporterSpi's because it are still not-production ready. There
>>> are some issues with it and usage of ReadOnlyMetricRegistry interface
>>> just one of them.
>>>
>>> The third, moving of MetricRegistry to the public API doesn't solve
>>> the problem because this interface exposes internal Metric interface
>>> implementations. So your PR is incomplete.
>>> Moreover, API of MetricRegistry is inconsistent. E.g. register(name,
>>> supplier, desc) method returns registered metric for some types and
>>> doesn't for other. register(metric) method is inconsistent in sense of
>>> metric naming. ReadOnlyMetricRegistry interface is redundant.
>>> MetricExporterSpi should be revised because it absolutely not
>>> intuitive because it requires ReadOnlyMetricRegistry and it's purpose
>>> is undefined.
>>>
>>> One more point. IEP-35 is still not fully implemented. Some things are
>>> not taken into account. Exporters expose metrics if they are disabled.
>>> JMX beans exposes values that don't confirm to best practices [1].
>>> Metrics of type lists are not metric at all. Ubiquitous merics lookup
>>> from hash map instead of usage reference for getting metrics values
>>> (it is just performance issue). Also IGNITE-11927 is not implemented
>>> which also changes interfaces significantly.
>>>
>>> Let's just admit that the implementation is immature and must be moved
>>> to the internal packages.
>>>
>>> And because we already merged partially implemented IEP to the master
>>> branch we *must move all currently public APIs to the internal API*
>>> while it will not be ready for publication.
>>>
>>> And the last but not least. What is happening indicates a immature
>>> development process which must be revised. I don't want discuss it in
>>> this thread but we must not allow merge of change to the master branch
>>> before it will completed, that is we must use feature branches for
>>> full IEP not only for particular tickets. And also we should
>>> reformulate IEP process in order to avoid things like this.
>>>
>>> [1] https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html
>>>
>>> On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков <[hidden email]> wrote:
>>>>
>>>> Alex.
>>>>
>>>> OK, I may leverage your experience and create pure Java API.
>>>> Ticket [1] created.
>>>>
>>>> But, personally, I don’t agree with you.
>>>> Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages.
>>>> Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users.
>>>>
>>>> You can just see count of the tests we just mute on the TC.
>>>>
>>>> Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3]
>>>>
>>>> [1] https://issues.apache.org/jira/browse/IGNITE-12553
>>>> [2] https://github.com/apache/ignite/pull/7269
>>>> [3] https://issues.apache.org/jira/browse/IGNITE-12552
>>>>
>>>>
>>>>> 17 янв. 2020 г., в 12:12, Alexey Goncharuk <[hidden email]> написал(а):
>>>>>
>>>>> Nikolay,
>>>>>
>>>>> Why do you think this is a wrong usage pattern? From the top of my head,
>>>>> here is a few cases of direct metric API usage that I know are currently
>>>>> being used in production:
>>>>> * A custom task execution scheduling service with load balancing based on
>>>>> utilization metrics readings from Java code
>>>>> * Cleanup task trigger based on metrics readings
>>>>> * A custom health-check endpoint for an application with an embedded
>>>>> Ignite node for Kubernetes/Spring Application/etc
>>>>
>>