>> 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.
We don't know anything about development processes our users. I can admit that process could require that deprecated methods/APIs are not allowed for example. >> 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: I understand this. But I'm not Ignite user, I'm Ignite developer. The key moment in my message *from user stand point*. From my point of view it is very not intuitive solution and this interface is redundant. >> Actually not. We have statisticsEnabled for caches for example. There are >> other entities with such flag. > They still works as expected. Actually not. I fixed many such issues during IGNITE-11927 implementation. >> 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. I talk not about ability. I talk about ignored existing functionality. So scope is not case here. >> 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. Actually list is not metric at all as I already told. On Fri, Jan 17, 2020 at 5:26 PM Николай Ижиков <nizhi...@apache.org> wrote: > > 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 > >>>> > >> >
