Skip to content

Jmx demo#2129

Merged
dbwiddis merged 7 commits into
oshi:masterfrom
SalvadorRomo:jmx-demo
Oct 8, 2022
Merged

Jmx demo#2129
dbwiddis merged 7 commits into
oshi:masterfrom
SalvadorRomo:jmx-demo

Conversation

@SalvadorRomo

Copy link
Copy Markdown
Contributor

This is just demo for JMX hope you can find it useful Regards!

@SalvadorRomo SalvadorRomo mentioned this pull request Jul 19, 2022
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/demo/Client.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/mbeans/BaseBoard.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/mbeans/BaseBoard.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/mbeans/BaseBoard.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/JMXOshiAgentServer.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/JMXOshiAgentServer.java Outdated
@dbwiddis

Copy link
Copy Markdown
Member

Looks like Sonatype and the CI is giving you some stuff to work with, so I'll hold off on further review.

But can you rebase onto master and make sure you keep the final solaris GHA file as it currently is on the master branch?

@SalvadorRomo

Copy link
Copy Markdown
Contributor Author

ready!! This last commit now has the changes without conflict 87858b8

@lgtm-com

lgtm-com Bot commented Jul 19, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 87858b8 into 0df2e9e - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

@lgtm-com

lgtm-com Bot commented Jul 19, 2022

Copy link
Copy Markdown

This pull request introduces 1 alert when merging 02c2929 into 0df2e9e - view on LGTM.com

new alerts:

  • 1 for Dereferenced variable may be null

Comment thread oshi-demo/src/main/java/oshi/demo/jmx/ContextRegistrationPlatform.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/JMXOshiAgentServer.java Outdated
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/JMXOshiAgentServer.java
Comment thread oshi-demo/src/main/java/oshi/demo/jmx/mbeans/BaseBoard.java Outdated
*/
package oshi.demo.jmx.mbeans;

public interface BaseBoardMBean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if rather than manually writing these MBeans you can use Jackson's ObjectMapper to accomplish the same purpose. Otherwise that's a ton of boilerplate code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh got it, will work over it, to use Jacksons ObjectMapper

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've got a question, do you want an ObjectMapper builder to create these interface ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Already chnaged all the others comments, the only one pending is this one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, busy today.

No specific suggestion here. I'm just thinking from the experience of the oshi-json artifact that maintaining a whole "mirror" of the API is a lot of work. I was hoping ObjectMapper could directly communicate with JMX without coding up the beans.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, rethinking more about what you had proposed, Im thinking in using ASM with a maven steps in which we can grab all the interfaces on the corresponding classpath, parse those clases into JSON with object mapper and later create the MBeans dinamycaally , so when the strategy for regsitering MBeans is executed, can have those MBenas ref, will work on that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beacuese sadly for JMX we need interface with the suffix MBena and the DynamicMBean would have been a good option but we would need to mantain some enums to specify the operation on each interface and that would end up in the same problem

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's take a step back and make sure we're not trying to solve an X Y problem here.

If JMX interface is limited to creating MBean classes, and we want to reproduce the whole API, then that's a lot of duplicated boilerplate code. Any change to one API has to be made in two places. For the entire 3.x series I maintained the oshi-json tree alongside oshi-core and it was somewhat of a pain. I can imagine a similar issue with MBeans.

We also can trivially generate JSON with Jackson, and return that easily. There's a webserver demo project that gives an idea of capabilities.

So we should scale down and consider JMX only for a subset of the API.

We can still build a JMX interface but we might want to limit it to the stuff that changes regularly, like CPU usage, memory usage, and possibly some process monitoring stuff. For stuff that doesn't change often (or at all), we could just return a JSON string (possibly with a JMX object, or possibly via a separate REST API).

Another approach would be to do some value added to the monitoring. Metrics includes a JMX server and I thought it wold be a great idea. (See #249.) Unfortunately Metrics doesn't handle arrays well and doesn't appear to be actively maintained so it's probably not a good choice. But the idea of building long-running averages of key values and exposing them via JMX or a REST API seems like a great idea. Maybe we could just code up our own server that queries OSHI and calculates some of these metrics (similar to the thread for Windows load averages recently added.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well at the end I went with the approach of DynamicMBean and reflection, so it resloves the overhead of creating repetitive code. And for giving the users the capability to know what properties are in each MBena I offer an Interface for giving them the list of props they can retrive for each MBEan

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm completely in for this effort about creating these servers relying on OSHI and from my perspective that I worked in System Managment systems, it is really hard to find these Open Source efforts well mantained and documented. Actually we struggle a lot with sblim-sfcb beacuse either you need to pay for support at the different Linux distos or you need to have c developers. and thks to you and all this comunity I came up with this awsome library that helped the businnes at that time. And with Converting this into complete server , the posibility are endlees, from either sending threshold event and more. And Regarding this effort, for overcoming the need to construct these interfaces I just implemented and approach with reflection, that way we can have the DynamicMBeams always in sync with watever change is made to the oshi interfaces

Comment thread .github/workflows/solaris.yaml
*/
package oshi.demo.jmx.mbeans;

public interface BaseBoardMBean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which option do you recommend?

OSHI already has interfaces that define what gets returned, which is essentially how Jackson converts them to its object. The DynamicMBean approach sounds to me like a reasonable option, but you have a lot more experience here so I'd like to hear your thoughts.

@dbwiddis dbwiddis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Any way we can write up some short docs on how to use it?

Comment thread .github/workflows/solaris.yaml
*/
package oshi.demo.jmx.mbeans;

public interface BaseBoardMBean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's take a step back and make sure we're not trying to solve an X Y problem here.

If JMX interface is limited to creating MBean classes, and we want to reproduce the whole API, then that's a lot of duplicated boilerplate code. Any change to one API has to be made in two places. For the entire 3.x series I maintained the oshi-json tree alongside oshi-core and it was somewhat of a pain. I can imagine a similar issue with MBeans.

We also can trivially generate JSON with Jackson, and return that easily. There's a webserver demo project that gives an idea of capabilities.

So we should scale down and consider JMX only for a subset of the API.

We can still build a JMX interface but we might want to limit it to the stuff that changes regularly, like CPU usage, memory usage, and possibly some process monitoring stuff. For stuff that doesn't change often (or at all), we could just return a JSON string (possibly with a JMX object, or possibly via a separate REST API).

Another approach would be to do some value added to the monitoring. Metrics includes a JMX server and I thought it wold be a great idea. (See #249.) Unfortunately Metrics doesn't handle arrays well and doesn't appear to be actively maintained so it's probably not a good choice. But the idea of building long-running averages of key values and exposing them via JMX or a REST API seems like a great idea. Maybe we could just code up our own server that queries OSHI and calculates some of these metrics (similar to the thread for Windows load averages recently added.)

@SalvadorRomo

Copy link
Copy Markdown
Contributor Author

and regarding the doc will work on an md! hope I can get it done by the end of the day! Regards!!

@SalvadorRomo

Copy link
Copy Markdown
Contributor Author

done!

@dbwiddis dbwiddis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good, thanks! Codacy doesn't like some things that Spotless could probably fix. :)

I think this could be improved just a little bit with actual code samples (unless those are actually completed in the "demo" directory, in which case link to them!) For example at the top, you have "all you need to do is..." but you could actually code a small class to do that and display it inline. You can keep the "as an alternative" paragraph as is.

Similarly it looks like you have most of the code for a client already typed... can you wrap it in minimal class headers so that it works? Just the basics to make it work, with the "if..." paragraphs following advising how to extend and adapt the code. Again, if the "demo" folder has these complete classes then just link to them, no need to repeat the code.

Alternately just have some small classes that actually run the minimal code included in the project

I note we sill have the Baseboard mbean. Is that still needed? And I'm not sure I understand the WindowsStrategyRegistration file, which also looks Baseboard-specific.

@SalvadorRomo

Copy link
Copy Markdown
Contributor Author

Great!! Will work on those observations.

And regarding the WindowsStrategyRegistration, My idea was to have an strategy for each platform so it could specialize the registration of each MBean into each platform, expample: cuztomize the MBean domain to have an specific name as oshi-windows: or add more stuff specific to platform at the MBean registration. And in that class I just wrote only one Mbean registration that is the Baseboard, but what I had in mind is that strategy had all the MBeans being initialized as CPU and more, but just for simplicity reason I just declare the baseboard one.

And Regarding the Baseboard MBean, it is not a MBean at all. Is a Proxy class that is extending DynamicMBean, so whenever we want to get some attribute from the MBean we can just override the getAttribute and call the specific getter method on the actual oshi object by reflection. But At the end we still need that proxy beacuse is the only way we can register this mbean into the MBean server beacause of the DyniamicMBean extension applying the MBean convention.

@SalvadorRomo

SalvadorRomo commented Jul 23, 2022

Copy link
Copy Markdown
Contributor Author

Another note i forgot to mention about WindowsStrategyRegistration , If this server in the near future can make it to the core API, we can take advantge of going directly into the internal API to just get specific information regarding OS and Hadware without the need to use a SystemInfo or HadwareAbstractLyer object , or another option is to aslk clients that they need specify these object and get rid of this strategy and make a simply one because, already those object has reference to each platform.

@dbwiddis dbwiddis added the hacktoberfest Issues we're happy for new #Hacktoberfest2020 participants to do label Oct 7, 2022
@sonatype-lift

sonatype-lift Bot commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

⚠️ 26 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@dbwiddis dbwiddis force-pushed the jmx-demo branch 6 times, most recently from acc6636 to 6587b30 Compare October 7, 2022 21:16
@dbwiddis dbwiddis force-pushed the jmx-demo branch 2 times, most recently from da1147e to 8299eab Compare October 7, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hacktoberfest Issues we're happy for new #Hacktoberfest2020 participants to do

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants