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

Add instrumentation for Tomcat webapp classloading #741

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

mar-kolya
Copy link
Contributor

@mar-kolya mar-kolya commented Feb 28, 2019

This is really the same thing we already do for OSGi and jboss.

The problem is that tomcat (and other app servers) have classloaders that do not delegate to parent.
This causes problems,
for example in case of tomcat things like GlobalTracer were loaded twice: ones by bootstrap and once by app loader. This resulted in having to separateGlobalTracers in place where it should have been just one.

@mar-kolya mar-kolya added this to the 0.25.0 milestone Feb 28, 2019
@mar-kolya mar-kolya added the inst: others All other instrumentations label Feb 28, 2019
@mar-kolya mar-kolya force-pushed the mar-kolya/tomcat-classloadin-instrumentation branch from 99fa027 to bc00349 Compare February 28, 2019 16:07
@tylerbenson
Copy link
Contributor

I don't understand the purpose of this... please add some comments in the code and update the PR description.

@mar-kolya mar-kolya force-pushed the mar-kolya/tomcat-classloadin-instrumentation branch from bc00349 to aa031cc Compare February 28, 2019 17:58
@mar-kolya
Copy link
Contributor Author

Added a comment into the code as well

Copy link
Contributor

@tylerbenson tylerbenson left a comment

Choose a reason for hiding this comment

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

I think this is ok. The difference with OSGi and JBoss is for those we are modifying configuration, right? This is changing the classloader behavior via bytecode changes

@mar-kolya
Copy link
Contributor Author

mar-kolya commented Feb 28, 2019

Well, kind of, yes. The goal is still the same: make app classloader load these classes off bootstrap.
With OSGi it is slightly more involved than just configuration change. With Tomcat it looks like there is no configuration for that at all.

@mar-kolya mar-kolya merged commit 3029d3d into master Feb 28, 2019
@mar-kolya mar-kolya deleted the mar-kolya/tomcat-classloadin-instrumentation branch February 28, 2019 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inst: others All other instrumentations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants