So the proposed change will not actually fix the issue with metrics,
because it's still possible to get a local instance via the service()
method. At the same time, the change removes an existing performance
optimization.
Let's figure out how to fix the actual problem. If the *only* way to have
metrics is to have a proxy, then this should be the only way to interact
with a service. In that case, we need to do something with the service()
method (deprecate it?). Or probably there are other ways to fix metrics?
-Val
On Thu, Jan 20, 2022 at 3:32 AM Vladimir Steshin <vlads...@gmail.com> wrote:
> Yes. Invocations from direct reference are not measured. This method
> in the javadoc:
>
>
> /* <b>NOTE:</b> Statistics are collected only with service proxies
> obtaining by methods like/
>
> /* {@link IgniteServices#serviceProxy(String, Class, boolean)} and won't
> work for direct reference of local/
>
> /* services which you can get by, for example, {@link
> IgniteServices#service(String)}./
>
>
> On 20.01.2022 00:20, Valentin Kulichenko wrote:
> > BTW, there is also the service() method that can only return an instance
> > and never returns a proxy. Does it corrupt the metrics as well?
> >
> > -Val
> >
> > On Wed, Jan 19, 2022 at 1:09 PM Valentin Kulichenko <
> > valentin.kuliche...@gmail.com> wrote:
> >
> >> Maxim,
> >>
> >> The reason I'm asking is that I don't really understand how client side
> >> mechanics affect server side metrics (number of executions and their
> >> durations). I feel that we might be fixing a wrong problem.
> >>
> >> Could you elaborate on why we count metrics incorrectly when
> >> the serviceProxy() returns an instance of a local service instead of an
> >> actual proxy?
> >>
> >> -Val
> >>
> >> On Tue, Jan 18, 2022 at 11:32 PM Maksim Timonin<timoninma...@apache.org
> >
> >> wrote:
> >>
> >>> Hi, guys!
> >>>
> >>>> this is not a good idea to change the behavior of serviceProxy()
> >>> depending on statistics
> >>>
> >>> I think that the patch doesn't change the behavior of `serviceProxy()`.
> >>> This method promises a proxy and it actually returns it. The fact that
> >>> `serviceProxy()` can return non-proxy objects is an internal Ignite
> >>> optimization, and users should not rely on this, there is a separate
> >>> method
> >>> `service()` for that.
> >>>
> >>>> What are the metrics that are being affected by this?
> >>> Only service metrics, that calculates duration of service execution.
> Check
> >>> this ticket [1]
> >>>
> >>> [1]https://issues.apache.org/jira/browse/IGNITE-12464
> >>>
> >>>
> >>> On Wed, Jan 19, 2022 at 1:22 AM Valentin Kulichenko <
> >>> valentin.kuliche...@gmail.com> wrote:
> >>>
> >>>> What are the metrics that are being affected by this?
> >>>>
> >>>> -Val
> >>>>
> >>>> On Tue, Jan 18, 2022 at 3:31 AM Вячеслав Коптилин <
> >>>> slava.kopti...@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> Hello Igniters,
> >>>>>
> >>>>> IMHO, this is not a good idea to change the behavior of
> serviceProxy()
> >>>>> depending on statistics (enabled/disabled). It seems counterintuitive
> >>> to
> >>>>> me.
> >>>>> Perhaps, we need to introduce a new method that should always return
> a
> >>>>> proxy to the user service.
> >>>>>
> >>>>> Thanks,
> >>>>> Slava.
> >>>>>
> >>>>>
> >>>>> вт, 28 дек. 2021 г. в 13:57, Pavel Pereslegin<xxt...@gmail.com>:
> >>>>>
> >>>>>> Hi!
> >>>>>>
> >>>>>> Agree with Maxim.
> >>>>>>
> >>>>>> It seems to me quite normal to return a proxy for a local instance
> >>> in
> >>>>>> the case when the user has explicitly enabled statistics collection
> >>> in
> >>>>>> the service settings. Those. by default, we do not change the
> >>> behavior
> >>>>>> and if the collection of metrics is not needed, a local instance
> >>> will
> >>>>>> be returned. And I also think the javadoc should be changed to
> >>> reflect
> >>>>>> the new behavior.
> >>>>>>
> >>>>>> So, I'm for 1 + 3.
> >>>>>>
> >>>>>> вт, 28 дек. 2021 г. в 10:51, Maksim Timonin <
> >>> timoninma...@apache.org>:
> >>>>>>> Hi!
> >>>>>>>
> >>>>>>> I agree that users shouldn't expect a non-proxy when invoking the
> >>>>>>> `IgniteServices#serviceProxy()` method. I think it's up to Ignite
> >>> to
> >>>>>> return
> >>>>>>> a non-proxy instance here as possible optimisation. But users
> >>> have to
> >>>>> use
> >>>>>>> interfaces in any case. There is the `IgniteServices#service()`
> >>>> method
> >>>>>> for
> >>>>>>> explicit return of local instances.
> >>>>>>>
> >>>>>>> With enabling of metrics we can break users that explicitly
> >>>>>>> use `#serviceProxy` (proxy!), and then explicitly cast it to an
> >>>>>>> implementation class. In this case such users will get a runtime
> >>>>>> exception.
> >>>>>>> I think we can write a good javadoc for
> >>>>>>> `ServiceConfiguration#setEnableMetrics()`, it should mention that
> >>> it
> >>>>>> works
> >>>>>>> only with proxy, and it doesn't collect metrics with non-proxy
> >>> usages
> >>>>>> with
> >>>>>>> `IgniteService#service()`.
> >>>>>>>
> >>>>>>> So, I propose to proceed with two solutions - 1 and 3: fix docs
> >>> for
> >>>>>>> `#serviceProxy()` and provide detailed javadocs
> >>>>>>> for `ServiceConfiguration#setEnableMetrics()`.
> >>>>>>>
> >>>>>>> If some users will enable metrics (even with such docs!) and will
> >>> be
> >>>>>> using
> >>>>>>> casting proxy(!) to an implementation, then they will get a
> >>> runtime
> >>>>>>> exception. But I believe that it is an obvious failure, and it
> >>> should
> >>>>> be
> >>>>>>> fixed on the user side.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On Mon, Dec 27, 2021 at 10:26 PM Vladimir Steshin <
> >>>> vlads...@gmail.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Hi, Igniters.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I'd like to suggest modifying
> >>> `/IgniteServices.serviceProxy(String
> >>>>>> name,
> >>>>>>>> Class<? super T> svcItf, boolean sticky)/` so that it may return
> >>>>> proxy
> >>>>>>>> even for local service. Motivation: service metrics [1]. To
> >>> measure
> >>>>>>>> method call we need to wrap service somehow. Also, the method
> >>> name
> >>>>> says
> >>>>>>>> `proxy`. For local one we return direct instance. Misleading.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Solutions:
> >>>>>>>>
> >>>>>>>> 1) Let’s return a proxy (`/GridServiceProxy/`) once we need.
> >>> Let’s
> >>>>>>>> change the javadoc to say `@return Proxy over service’. Simply
> >>>> works.
> >>>>>>>> Pros: invocation like `/MyServiceImpl svc =
> >>>> serviceProxy(«myservice»,
> >>>>>>>> MyService.class)`/ would fail. Wierd usage to me. But possible.
> >>>>>>>>
> >>>>>>>> 2) Introduce a new method with forced-proxy flag
> >>>>>>>> like`IgniteSerives.serviceProxy(…, boolead focedProxy)`. And
> >>> add a
> >>>>>>>> warning to other service-obtain methods like: «`/You enabled
> >>>> service
> >>>>>>>> metrics but it doesn’t work for local service instanse. Use
> >>>>>> forced-proxy
> >>>>>>>> serviceProxy(…, boolean forcedProxy)/` Pros: we’ve already have
> >>>> about
> >>>>>>>> 5-6 methods to get services in `/IgniteServices/`. I doubt we
> >>> need
> >>>>> one
> >>>>>>>> more.
> >>>>>>>>
> >>>>>>>> 3) Fix the documentation so that it tells the service metrics
> >>> would
> >>>>>> work
> >>>>>>>> only with proxy. Pros: service metrics just won’t work for local
> >>>>>>>> services. Suddenly.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> My vote is for #1: let's use a proxy. WDYT?
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> [1]https://issues.apache.org/jira/browse/IGNITE-12464
> >>>>>>>>
> >>>>>>>>
