Skip to content
This repository has been archived by the owner on Mar 25, 2018. It is now read-only.

#1473: Add JMX support. #1474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

#1473: Add JMX support. #1474

wants to merge 1 commit into from

Conversation

iocanel
Copy link
Member

@iocanel iocanel commented Apr 1, 2013

This is a first attempt on adding jmx support to jclouds and I am creating a pull request for feedback.

What this pull request adds:

i) A Management Context, which keeps tracks of jclouds managed beans, contexts and mbean servers.
ii) Converter functions for converting objects to open types (So that the jclouds mbeans can be used from jconsole, visualvm without requiring the addition of jclouds client jars).
iii) Annoatations that mark which classes are convertible to open types and how they should be converted.
iv) Mbeans for displaying Apis, Providers and Contexts. Also MBeans for compute services and blobstores.

@buildhive
Copy link

Adrian Cole » jclouds #1158 SUCCESS
This pull request looks good
(what's this?)

@mattstep
Copy link
Member

mattstep commented Apr 1, 2013

Please break this pull request up

@mattstep
Copy link
Member

mattstep commented Apr 1, 2013

Looking through the commit, this looks vastly overcomplicated and highly intrusive. I'm not sure when I can actually review this entire commit, but I was hoping from our email thread to have something a bit simpler that isn't as invasive in the existing code base. I feel that right now we're struggling with a large code base that's perpetually growing in ways that are not necessarily in line with the primary interests of the project. It would be far better if we could simplify this.
The tests also appear relatively minimal for the amount of code being added. I also know from experience that most software engineers don't know much about jmx or how to use it. Can you write up something on how this works to begin with? It would be terrible to add this much complexity with low user adoption.

@iocanel
Copy link
Member Author

iocanel commented Apr 1, 2013

Matt, its was pretty clear from the mailing list that we want something simple at least at first. As I mentioned in the mailing list, I am not confident myself that the direction I took is the right one, this why I started the discussion in the first place. I created the pull request, so that I can get some feedback and see how we are going to proceed.

So a few words about the contents of this commit:

The core of this commit is the addition of 3 MBeans:

i) A Core Managmenet MBean for displaying: Apis, Proivders and Contexts.
ii) A Compute Service MBean, which exposes some of the ComputeService operations to JMX. [2]
iii) A BlobStore MBean, which exposes some of the BlobStore operations to JMX. [3]

The first one is registered once and the following ones are registered once per relevant context.

The registration of the mbeans is handled by ManagementContext . The management context can be passed to the ContextBuilder and get bind to the ContextImpl. Each context created will register itself after its creation and unregister itself when it closes itself. (e.g. Compute Service Context).

The ManagementContext is also responsible for reacting to the MBeanServer lifecycle.

The rest of the commit is dedicated on converting jclouds internal objects to open type objects. Why? To be able to use the MBeans from clients, without requiring to add jclouds jars to them. This is mostly usefull when using tools like JConsole or VisualVM.

So how this works? I added 2 annotations the ManageType and ManagedAttribute. The ManagedType is used to annotate classes, that we want to convert to OpenType. The ManagedAttribute is used to choose which properties/methods of the object we want to be added as part of the converted type.

These annotations have been used to annotate objects like: Location, Image, Context, ApiMetadata, ProviderMetadata etc.

The 3 MBeans themselves are using a converter functions that converts @ManagedType annotated objects to CompositeData and Iterables of @ManageType objects to TabularData (CompositeData & TabularData are the OpenTypes that we are using).

The conversion functions are using a model class which contains all the information provided by the annotations, like property names, description etc and perform the conversion.

And this is pretty much it.

The only thing that is somewhat intrusive is binding the management context object to the context itself (any ideas or suggestions are more than welcome). All the rest of the existing code is only affected by using annotations.

Now regarding the conversion part, I am not aware of tools or framework that provides a solution for that. Other projects I've seen try to solve this problem by explicitly implementing a converter class per type or implementing the CompositeDataView interface, which is imho harder to maintain and more intrusive (than using annotations + reflection).

@demobox
Copy link
Member

demobox commented Apr 1, 2013

All the rest of the existing code is only affected by using annotations.

I'm sure we could spend many long hours on fun conversations about current Java usage of annotations, but I think it's fair to say that even if classes are "only" affected by annotations they are affected nevertheless, which makes for quite an "intrusive" change simply in terms of the number of files affected.

I know we're discussing a (new) jclouds-internal annotation and not an external class here, but the fact remains that we would end up with a bunch of new metadata all over the jclouds code that relates only to one specific concern (JMX exposure).

Is there some way to find what the converters need to know - which classes to managed, for example - simply based on the type system, e.g. by looking at the interfaces implemented?

@demobox
Copy link
Member

demobox commented Apr 1, 2013

The only thing that is somewhat intrusive is binding the management context object to the context itself (any ideas
or suggestions are more than welcome).

Could this be solved by some kind of "context creation event callback"? That could leave the linking up of the contexts to JMX in an optional JMX layer (a driver?) without affecting the core.

@iocanel
Copy link
Member Author

iocanel commented Apr 1, 2013

@demobox: The context event callback sounds a good idea to me, as a solution to avoid cluttering the context implementations.

If we remove the annotations, we will loose the description of the managed attributes (no biggie), we would have more clutter (in what we expose), like things that make no sense to expose (builders, streams etc) and finally we would have some issues (possibly solvable) with cyclic references etc. Its doable but it might be just better to create a custom coverter per class (in terms of having a better handling the output).

@mattstep
Copy link
Member

mattstep commented Apr 2, 2013

Agree with @demobox, I'll take a deeper look tomorrow. Wanna take a stab at trying to avoid the new annotations?

@iocanel
Copy link
Member Author

iocanel commented Apr 2, 2013

@mattstep: I'll try to take a stub at it.

@iocanel
Copy link
Member Author

iocanel commented Apr 3, 2013

@demobox, @mattstep: Using an "context event callback" works nice.

Removing the annotations and doing everything through the type system, requires enriching the conversion functions with even more cases (since we are processing all properties/methods and not just annotated ones). It doesn't look good and it definitely doesn't worth the effort.

If we completely scrap the opentype conversion, we will need to have the types we expose at least being Serializable, if we would like ppl to use remote jmx clients. From the commit log I see that we have removed the Serializable from almost all the jclouds types.

@demobox
Copy link
Member

demobox commented Apr 3, 2013

Using an "context event callback" works nice.

Nice!

It doesn't look good and it definitely doesn't worth the effort.

OK. Quick step back here: could you describe which "things" (whether types or otherwise) we're interested in displaying/exposing through JMX? Perhaps someone can come up with a smart suggestion for how to identify those things without just looking at the types...

Thanks!

@iocanel
Copy link
Member Author

iocanel commented Apr 3, 2013

I think that we would need to expose at least:

i) Available Apis & Providers
ii) Contexts

For each Compute Service:
i) Images
ii) Locations
iii) Hardware
iv) Nodes
v) Operations for creating, deleting, running scripts on nodes.

For each BlobStore:
i) Locations
ii) Containers
iii) Blobs
iv) Misc Metadata for the above
v) Operations to create, destroy etc containers and blobs.

Others have mentioned that they would also like to expose statistics etc.

@demobox
Copy link
Member

demobox commented Apr 3, 2013

i) Available Apis & Providers
ii) Contexts

This would seems to be a bounded list of known types? I guess you would get and display all ApiMetadata instances. They normally contain only basic types, I'm guessing. Getting at all open contexts would be more interesting...do we keep such a registry?

For each Compute Service:
i) Images
ii) Locations
iii) Hardware
iv) Nodes
v) Operations for creating, deleting, running scripts on nodes.

Would it be enough to simply expose the information that ComputeService returns, or are you after the provider-specific details here too (I suspect so)? In that case, how many non-basic types are involved here? I'm guessing Image, NodeMetadata and the others, as well as any argument types for the operations on ComputeService. How many non-basic types are there?

For each BlobStore:
i) Locations
ii) Containers
iii) Blobs
iv) Misc Metadata for the above
v) Operations to create, destroy etc containers and blobs.

How many additional non-basic types are we talking about here? E.g. Location would seem to be shared with compute...

@iocanel
Copy link
Member Author

iocanel commented Apr 5, 2013

@demobox: After some talk in irc. It was proposed to create a dto project in the labs. And since with your idea the current userbase is not affected by the management/jmx stuff I also added a jclouds-manangement project in the labs. Pull Request for the above: jclouds/jclouds-labs#24

@mattstep
Copy link
Member

mattstep commented Apr 8, 2013

Does this need to remain open? Can we close this now that we're looking at starting this work in labs?

@zack-shoylev
Copy link
Contributor

Seems like this should be closed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants