> 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 > >> >
