Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jmx adapter #431

Merged
merged 48 commits into from Jun 19, 2020
Merged

Jmx adapter #431

merged 48 commits into from Jun 19, 2020

Conversation

skarsaune
Copy link
Contributor

Attempt to create a MBeanServerConnection proxy using jolokia as the actual backend, with fairly complete test coverage of what you get out of the box in a Java VM. More third party MBeans can be added to increase test coverage of problematic cases.
Tries to mimic exceptions and response values as closely as possible to JSR-160 connection.
Introduces a new internal dependency to jolokia-core from test to compile in order to reuse mapping to OpenType,
if that dependency is a problem the new adapter can be split out to a separate module.
Also wondering if one should automate tests with different old releases of the Jolokia JVM agent and possibly different JVM versions in github actions, to test permutations of clients and server.

@skarsaune skarsaune requested a review from rhuss February 14, 2020 09:47
@rhuss
Copy link
Member

rhuss commented Feb 14, 2020

Hey Martin, thanks a tons for all your work and dedication ! Let me try to jump on this, but this might take a bit. Hope to come to this next week, but there's a lot work on my plate these days ;-)

But that's all really very cool. Were you able to run this adapter in e.g. jconsole or visualvm ?

@skarsaune
Copy link
Contributor Author

Hey Martin, thanks a tons for all your work and dedication ! Let me try to jump on this, but this might take a bit. Hope to come to this next week, but there's a lot work on my plate these days ;-)

But that's all really very cool. Were you able to run this adapter in e.g. jconsole or visualvm ?

Looking into running in jmc, but that is a big task in itself. No need to rush, I can try the jmc integration with a local snapshot, and it will probably result in adjustments as well.

… and jvisualvm by adding to classpath (DOES NOT work particularly well at this stage)
@skarsaune
Copy link
Contributor Author

Have been testing directly in jconsole and jvisualvm by adding to classpath and using jmx url for example service:jmx:jolokia://localhost:9091/hawtio/jolokia/
plenty of stuff needs ironing out, plenty of adjustments probably. If it turns out to be not feasible, may shift focus to flight recordings from jmc as that was the original goal

@rhuss
Copy link
Member

rhuss commented Feb 18, 2020

@skarsaune no worries, don't get lost. I haven't heard many requests about jconsol / jsvisualvm recently, so please focus what you need for your use case. If jvisualvm or jconsole support is requested, it still can be added later.

@rhuss
Copy link
Member

rhuss commented Feb 18, 2020

wrt to a review, I can't jump on it until the over next week (busy this week, pto next). So please ping me again in case I should forget.

Thanks !

@skarsaune
Copy link
Contributor Author

I will stay on the JConsole track a few more days, it is a nice real world example and the code is easily accessible within the JDK, so it is a fruitful excercise anyway. With regards to review I would hold off a bit, things may change quite a lot. Thought I could label it as draft, but do not find that option. Anyway I can notify when I believe things are more stable.

…vms, and allow values in payload not covered
@rhuss
Copy link
Member

rhuss commented Feb 29, 2020

Good timing, just back from PTO ;-) (well, if it's good timing we will see when the e-mail backslash will he me :)

I will try to find a slot next week to jump on a review ...

@skarsaune
Copy link
Contributor Author

Should be OK to review now. Works with JMC as well. Playlist: https://www.youtube.com/watch?v=PDf2mqxOeMk&list=PLj8NeDWnVAdAu5X5hzioxLdSOhYSvrXTF , feel free to tweet however you like as a list or separately.

@skarsaune skarsaune mentioned this pull request Mar 23, 2020
@rhuss
Copy link
Member

rhuss commented Mar 30, 2020

Sorry, didn't make it last week :( So next try is this week ....

Copy link
Member

@rhuss rhuss left a comment

Choose a reason for hiding this comment

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

Thank @skarsaune for this awesome PR and equally for your patience ;)

It looks good to me, with some minor mostly cosmetic comments and suggestions. If you could address these in this week we can merge it (hint: I have PTO for the rest of the week :)

The only thing missing then is documentation, I will have a look where to best hook in documentation for the JmxAdapter.

thanks!

Map<String, Object> mergedEnv = mergedEnvironment(env);
String internalProtocol = "http";
if (String.valueOf(this.serviceUrl.getPort()).endsWith("443") || "true"
.equals(mergedEnv.get("jmx.remote.x.check.stub"))) {
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason why the environment is called like this? For me it's not obvious that this environment variable relates to select https as protocol. If this is not a standard variable, maybe we can choose some name which indicates its usage ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you refer to "jmx.remote.x.check.stub"? It is used by JConsole. I can create a named constant if that is the case?

Copy link
Member

Choose a reason for hiding this comment

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

I see. I'm ok with using it literally here, maybe a comment would be nice as it is not obvious that this is defined bei JConsole.

skarsaune and others added 6 commits May 27, 2020 16:50
…/ToOpenTypeConverter.java

Co-authored-by: Roland Huß <rhuss@redhat.com>
…/RemoteJmxAdapter.java

Co-authored-by: Roland Huß <rhuss@redhat.com>
…/RemoteJmxAdapter.java

Co-authored-by: Roland Huß <rhuss@redhat.com>
…/RemoteJmxAdapter.java

Co-authored-by: Roland Huß <rhuss@redhat.com>
…/RemoteJmxAdapter.java

Co-authored-by: Roland Huß <rhuss@redhat.com>
@rhuss
Copy link
Member

rhuss commented Jun 19, 2020

@skarsaune thanks a lot ! Looks we are ready to merge now :)

Wrt/ the documentation we can add a followup PR, so that we then can make a release.

Thanks again for your awesome work!

@rhuss rhuss merged commit e969126 into jolokia:master Jun 19, 2020
Assert.assertEquals(
this.adapter.getDefaultDomain(), nativeServer.getDefaultDomain(), "Default domain");

Assert.assertEquals(this.adapter.agentVersion, "1.6.2");
Copy link
Member

Choose a reason for hiding this comment

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

@skarsaune I think this will give us some headaches when releasing. We should either remove this test or make it at parameterised with a property from the pom.xml

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

Successfully merging this pull request may close these issues.

None yet

2 participants