Re: Static hierarchy in jmx tree

[Alexei Scherbakov <al...@gmail.com>]

I'm ok to use either classloader or node id.

I'm ok to always use instance name or consistent/node id to keep the same
hierarchy levels (in that order: instanceName -> consistentId -> nodeId.

It's not ok to have the property IGNITE_MBEAN_STATIC_HIERARCHY - actually
it's not always static, because node id changes on each restart.

Instead I would prefer to make IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false by
default.

This is a dangerous change which most likely will break node startup in
application containers (using different classloaders), so I'm strongly
disliking what.

Instead, is it possible to write a template using some kind of regexp and
ignoring classloader id ? To avoid issues with multiple templates I suggest
always output classloaderId in hierarchy.


вс, 13 июн. 2021 г. в 16:01, Igor Akkuratov <ak...@mail.com>:

> Stanislav, thank you for your response.
> I am fine with the option IGNITE_MBEAN_STATIC_HIERARCHY=true, is looks
> fair to give the opportunity to use the old behavior. But I do not
> understand why there is a classloader, I don't know any reason for that,
> whenever there is already a unique id - node id. It looks much more
> appropriate for me to use node id instead of classloader. So my idea is to
> use either instance name or consistentId or  nodeId is there any cons why
> it's better to do that your way?
>
> > Sent: Monday, June 07, 2021 at 4:21 PM
> > From: "Stanislav Lukyanov" <st...@gmail.com>
> > To: dev@ignite.apache.org
> > Subject: Re: Static hierarchy in jmx tree
> >
> > I guess the problem with this thread is that, on one hand, virtually
> anyone would support the change because no one likes the class loader IDs
> in JMX.
> > On the other hand, this is a breaking change and is kind of scary to do
> :)
> >
> > > I am not sure that I got you. If we that is all about containers like
> docker, then there is no any problem here.
> >
> > I believe Alexei means application containers, like WebSphere.
> >
> > It seems to me that the entire IGNITE_MBEAN_APPEND_CLASS_LOADER_ID
> behavior solves one very narrow use case:
> > running multiple apps, each with an Ignite client, each with no instance
> name set; in this case it is indeed awkward to see that your app with
> instance name = null can't start because there is already a different app
> with instance name = null.
> >
> > How about the following behavior?
> > - Always use EITHER instance name or class loader ID
> > - If the node doesn't have an instance name set, use its class loader ID
> > -- This way multiple client nodes in multiple apps in the same container
> can work together
> > - If the node does have an instance name set, use it without class
> loader ID
> > -- This way people with instance names set will get nice stable JMX
> hierarchies
> > - In both cases, the ObjectName will be
> > -- org.apache.ignite.<XYZ>.<METRICS> where <XYZ> is either the instance
> name of the class loader ID. In both cases, the metrics are on the same
> nesting level which is good I guess..
> >
> > Since this is not a core API I think it's alright to break the behavior
> in one minor release - given that there is a fallback option.
> > With that in mind, I'd rather have a new option like
> IGNITE_MBEAN_STATIC_HIERARCHY
> > - IGNITE_MBEAN_STATIC_HIERARCHY=true (default): the new behavior is in
> effect, IGNITE_MBEAN_APPEND_CLASS_LOADER_ID is ignored
> > - IGNITE_MBEAN_STATIC_HIERARCHY=false: the old behavior is in effect
> >
> > Thanks,
> > Stan
> >
> > > On 10 May 2021, at 23:04, Igor Akkuratov <ak...@mail.com> wrote:
> > >
> > > Alexei, thank you for your response.
> > >
> > >> 1. I don't understand why setting
> > >> IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not
> a
> > >> solution to your issue ?
> > >> Just put it in the documentation and this should do fine.
> > >
> > > It could be a solution for me, but I believe that such approach is a
> pain for a lot of users. Anyone who want to monitor Ignite node via JMX
> have to use both of options which have to be specified in different places.
> > >
> > >> 2. Removing the classloader id can break the template working in the
> > >> container environment, where the instances with the same name are
> > >> instantiated using different classloaders.
> > >> How is this scenario supposed to work with a single template ?
> > >
> > > I am not sure that I got you. If we that is all about containers like
> docker, then there is no any problem here. Each application have to be
> started in separate container with there own network, so yes - jmx tree is
> the same, but there are from different hosts. So right now there are few
> scenarios:
> > > - right now JMX is turned off by default, so there is no problem
> > > - if you want to use jmx you have to turn it on manually and it would
> work. After my patch consistent id in case of persistent or node id in
> other cases would be used.
> > > - if you want to specify instance name you can chose different names
> for different instances or separate them to different jvm.
> > > - if you want to use the previous logic with class loader id, option
> MBEAN_APPEND_CLASS_LOADER_ID is still available.
> > >
> > >
> > >> 3. Your patch introduces breaking change. This can be done only in two
> > >> steps: release N deprecated the behavior, release N + 1 changes the
> > >> behavior, according to the new rules.
> > >
> > > Ok. If we take a decision, I will do that.
> >
> >
>


-- 

Best regards,
Alexei Scherbakov

Re: Static hierarchy in jmx tree

[Igor Akkuratov <ak...@mail.com>]

Alexei,

>It's not ok to have the property IGNITE_MBEAN_STATIC_HIERARCHY - actually
>it's not always static, because node id changes on each restart.
 
I am fine to remove this property.

> Instead I would prefer to make IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false by
> default.

Already done in my PR.

> This is a dangerous change which most likely will break node startup in
> application containers (using different classloaders), so I'm strongly
> disliking what.

From my point of view user specified values like instanceName and consistantId or uniq nodeId should be enough for any case, could you please describe the case with application contained more detailed?

> Instead, is it possible to write a template using some kind of regexp and
> ignoring classloader id ? To avoid issues with multiple templates I suggest
> always output classloaderId in hierarchy.

It is possible to write template with regexp to detect metrics with non-static classloader in zabbix for example. But on every restart classloader will be another and zabbix would detect a metric as new, because metrics detection use full jmx path as key. In other monitoring systems that can work another way.



> Sent: Thursday, August 19, 2021 at 4:51 PM
> From: "Alexei Scherbakov" <al...@gmail.com>
> To: "dev" <de...@ignite.apache.org>
> Subject: Re: Static hierarchy in jmx tree
>
> I'm ok to use either classloader or node id.
> 
> I'm ok to always use instance name or consistent/node id to keep the same
> hierarchy levels (in that order: instanceName -> consistentId -> nodeId.
> 
> It's not ok to have the property IGNITE_MBEAN_STATIC_HIERARCHY - actually
> it's not always static, because node id changes on each restart.
> 
> Instead I would prefer to make IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false by
> default.
> 
> This is a dangerous change which most likely will break node startup in
> application containers (using different classloaders), so I'm strongly
> disliking what.
> 
> Instead, is it possible to write a template using some kind of regexp and
> ignoring classloader id ? To avoid issues with multiple templates I suggest
> always output classloaderId in hierarchy.
> 
> 
> вс, 13 июн. 2021 г. в 16:01, Igor Akkuratov <ak...@mail.com>:
> 
> > Stanislav, thank you for your response.
> > I am fine with the option IGNITE_MBEAN_STATIC_HIERARCHY=true, is looks
> > fair to give the opportunity to use the old behavior. But I do not
> > understand why there is a classloader, I don't know any reason for that,
> > whenever there is already a unique id - node id. It looks much more
> > appropriate for me to use node id instead of classloader. So my idea is to
> > use either instance name or consistentId or  nodeId is there any cons why
> > it's better to do that your way?
> >
> > > Sent: Monday, June 07, 2021 at 4:21 PM
> > > From: "Stanislav Lukyanov" <st...@gmail.com>
> > > To: dev@ignite.apache.org
> > > Subject: Re: Static hierarchy in jmx tree
> > >
> > > I guess the problem with this thread is that, on one hand, virtually
> > anyone would support the change because no one likes the class loader IDs
> > in JMX.
> > > On the other hand, this is a breaking change and is kind of scary to do
> > :)
> > >
> > > > I am not sure that I got you. If we that is all about containers like
> > docker, then there is no any problem here.
> > >
> > > I believe Alexei means application containers, like WebSphere.
> > >
> > > It seems to me that the entire IGNITE_MBEAN_APPEND_CLASS_LOADER_ID
> > behavior solves one very narrow use case:
> > > running multiple apps, each with an Ignite client, each with no instance
> > name set; in this case it is indeed awkward to see that your app with
> > instance name = null can't start because there is already a different app
> > with instance name = null.
> > >
> > > How about the following behavior?
> > > - Always use EITHER instance name or class loader ID
> > > - If the node doesn't have an instance name set, use its class loader ID
> > > -- This way multiple client nodes in multiple apps in the same container
> > can work together
> > > - If the node does have an instance name set, use it without class
> > loader ID
> > > -- This way people with instance names set will get nice stable JMX
> > hierarchies
> > > - In both cases, the ObjectName will be
> > > -- org.apache.ignite.<XYZ>.<METRICS> where <XYZ> is either the instance
> > name of the class loader ID. In both cases, the metrics are on the same
> > nesting level which is good I guess..
> > >
> > > Since this is not a core API I think it's alright to break the behavior
> > in one minor release - given that there is a fallback option.
> > > With that in mind, I'd rather have a new option like
> > IGNITE_MBEAN_STATIC_HIERARCHY
> > > - IGNITE_MBEAN_STATIC_HIERARCHY=true (default): the new behavior is in
> > effect, IGNITE_MBEAN_APPEND_CLASS_LOADER_ID is ignored
> > > - IGNITE_MBEAN_STATIC_HIERARCHY=false: the old behavior is in effect
> > >
> > > Thanks,
> > > Stan
> > >
> > > > On 10 May 2021, at 23:04, Igor Akkuratov <ak...@mail.com> wrote:
> > > >
> > > > Alexei, thank you for your response.
> > > >
> > > >> 1. I don't understand why setting
> > > >> IGNITE_MBEAN_APPEND_CLASS_LOADER_ID=false + igniteInstanceName is not
> > a
> > > >> solution to your issue ?
> > > >> Just put it in the documentation and this should do fine.
> > > >
> > > > It could be a solution for me, but I believe that such approach is a
> > pain for a lot of users. Anyone who want to monitor Ignite node via JMX
> > have to use both of options which have to be specified in different places.
> > > >
> > > >> 2. Removing the classloader id can break the template working in the
> > > >> container environment, where the instances with the same name are
> > > >> instantiated using different classloaders.
> > > >> How is this scenario supposed to work with a single template ?
> > > >
> > > > I am not sure that I got you. If we that is all about containers like
> > docker, then there is no any problem here. Each application have to be
> > started in separate container with there own network, so yes - jmx tree is
> > the same, but there are from different hosts. So right now there are few
> > scenarios:
> > > > - right now JMX is turned off by default, so there is no problem
> > > > - if you want to use jmx you have to turn it on manually and it would
> > work. After my patch consistent id in case of persistent or node id in
> > other cases would be used.
> > > > - if you want to specify instance name you can chose different names
> > for different instances or separate them to different jvm.
> > > > - if you want to use the previous logic with class loader id, option
> > MBEAN_APPEND_CLASS_LOADER_ID is still available.
> > > >
> > > >
> > > >> 3. Your patch introduces breaking change. This can be done only in two
> > > >> steps: release N deprecated the behavior, release N + 1 changes the
> > > >> behavior, according to the new rules.
> > > >
> > > > Ok. If we take a decision, I will do that.
> > >
> > >
> >
> 
> 
> -- 
> 
> Best regards,
> Alexei Scherbakov
>