Skip to content
New issue

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

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

Already on GitHub? # to your account

Pinpoint Jboss Plugin #2001

Merged
merged 4 commits into from
Oct 11, 2016
Merged

Pinpoint Jboss Plugin #2001

merged 4 commits into from
Oct 11, 2016

Conversation

suraj-raturi
Copy link
Contributor

@Xylus - Here raising PR for Jboss plugin, Please have a look and let us know if you see any issue
#972 Tested on Jboss EAP version - 6.4.x, We are in process of checking the compatibility with other versions as well.

c = classLoader.loadClass(className);
// Using different approach to load classes via classloader
// c = classLoader.loadClass(className);
c = Class.forName(className, false, classLoader);
Copy link
Member

Choose a reason for hiding this comment

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

Hello~ suraj-raturi
Why did this code change?
Tell me the reason to change.

Copy link
Contributor Author

@suraj-raturi suraj-raturi Aug 25, 2016

Choose a reason for hiding this comment

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

Hi @emeroad

Classloader can load classes via both the ways:

  1. classLoader.loadClass
  2. Class.forName

With classLoader.loadClass I was facing linkage error for example - "Caused by: java.lang.LinkageError: loader (instance of org/jboss/modules/ModuleClassLoader): attempted duplicate class definition for name: "com/navercorp/pinpoint/plugin/jdbc/mysql/MySqlConstants", while agent was instrumenting class in jboss environment.

I looked at the alternative ways to load classes in multi-threaded environment as the issue of linkage error has been reported by many (not specifically with Jboss) with classloader.loadClass.

I tried to load class with Class.forName, it worked for me without any issues for Jboss. To be absolutely sure that it won't break anything, I tested it with Tomcat environment as well and it worked fine.

Let me know your thoughts.

Thanks,
Suraj

Copy link
Member

Choose a reason for hiding this comment

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

Hello. @suraj-raturi
I have a good news.
We solved JDKLogger problem in jboss. #2026

We will solved Concurrent ClassLoading problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @emeroad ,

Thats good news, our approach while working for Jboss plugin was to go for minimum code change in existing Pinpoint project.
So, we choosed to solve the logger problem via defining logmanager property and logmanager jar in Jboss configuration.

Regarding Classloading issue, will be happy to be involve in the discussion and would like to contribute as well, if a more better approach from above code change in PlainClassLoaderHandler can be done.

Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Hi @suraj-raturi
I fixed the JBoss CL issue from #2117
But This PR need to modify setting.
plugins/jboss/README.md
-Djboss.modules.system.pkgs=com.navercorp.pinpoint
-Djboss.modules.system.pkgs=com.navercorp.pinpoint.bootstrap,
com.navercorp.pinpoint.common,
com.navercorp.pinpoint.exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @emeroad ,
I have updated the README file.
Thanks

@dawidmalina
Copy link
Contributor

Would be nice to see this plugin with pinpoint 1.6.0

@nstopkimsk nstopkimsk added this to the 1.6.0 milestone Oct 6, 2016
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 6, 2016
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 8, 2016
@emeroad
Copy link
Member

emeroad commented Oct 8, 2016

Hi @suraj-raturi
This PR branch has conflict.
Please resolve conflict.
Or Could you please give me maintainer permission

@suraj-raturi
Copy link
Contributor Author

Hi @emeroad ,
I have granted permission to allow edits from maintainers.
Thanks

@emeroad emeroad merged commit 2e4517a into pinpoint-apm:master Oct 11, 2016
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 11, 2016
 - add .gitignore & clover.license
emeroad added a commit that referenced this pull request Oct 11, 2016
 - add .gitignore & clover.license
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 11, 2016
 - avoid transform class collision (Tomcat plugin)
 - add emporary option : profile.jboss.enable=false (+1 squashed commits)
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 11, 2016
 - avoid transform class collision (Tomcat plugin)
 - add temporary option : profile.jboss.enable=false
emeroad added a commit that referenced this pull request Oct 12, 2016
 - avoid transform class collision (Tomcat plugin)
 - add temporary option : profile.jboss.enable=false
emeroad added a commit that referenced this pull request Oct 12, 2016
 - avoid transform class collision (Tomcat plugin)
 - add temporary option : profile.jboss.enable=false
Xylus added a commit to Xylus/pinpoint that referenced this pull request Oct 12, 2016
Xylus added a commit that referenced this pull request Oct 12, 2016
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 13, 2016
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 13, 2016
emeroad added a commit that referenced this pull request Oct 13, 2016
- fix testcase fail
emeroad added a commit to emeroad/pinpoint that referenced this pull request Oct 14, 2016
emeroad added a commit that referenced this pull request Oct 14, 2016
 - fix testcase fail
@Xylus
Copy link
Contributor

Xylus commented Dec 6, 2016

There is currently a bug in our ASM engine - You must set profiler.instrument.engine=JAVASSIST in pinpoint.config when using jboss plugin.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants