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 <ag...@apache.org> написал(а): > >> 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 Николай Ижиков <nizhi...@apache.org> 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 <ag...@apache.org> написал(а): >>> >>> 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 Николай Ижиков <nizhi...@apache.org> 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 <alexey.goncha...@gmail.com> >>>>> написал(а): >>>>> >>>>> 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 >>>> >>
