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

workaround for jira OSGi modules class loaders #2230

Closed
wants to merge 2 commits into from

Conversation

lpriima
Copy link
Contributor

@lpriima lpriima commented Dec 19, 2020

this workaround skips atlassian OSGi (apache felix) by its toString() implementation.

@lpriima lpriima marked this pull request as ready for review December 23, 2020 22:36
@lpriima lpriima requested a review from a team as a code owner December 23, 2020 22:36
@lpriima lpriima requested a review from mcculls December 23, 2020 22:43
@@ -56,6 +56,11 @@ public boolean matches(final ClassLoader cl) {
if (v != null) {
return v;
}
// workaround for jira osji modules class loaders:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please elaborate on why this is necessary.

Copy link
Contributor

@mcculls mcculls Jan 4, 2021

Choose a reason for hiding this comment

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

The root issue is that in OSGi the usual parent-child classloader association and visibility rules no longer apply. This can lead to instrumentation class-loading exceptions and log-spam, which as I understand this PR attempts to handle by simply turning off instrumentation for anything loaded by these classloaders.

I'm updating various parts of the tracer to handle class-lookup better when we know OSGi is being used, so this is more of a short-term workaround which we can hopefully drop very soon. If we did want to apply this workaround then I think we should explicitly list the names in canSkipClassLoaderByName rather than introduce another code path here.

Copy link
Member

Choose a reason for hiding this comment

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

If we did want to apply this workaround then I think we should explicitly list the names in canSkipClassLoaderByName rather than introduce another code path here.

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problematic atlassian classloaders class name are org.apache.felix.framework.BundleWiringImpl$BundleClassLoader. As far as understand in this workaround we don't want to skip all classloaders of this type. We only want to skip atlassian classloaders. These class loader toString() looks like com.atlassian.analytics.client [<some dynamic digits>] that's why the skip pattern is by prefix, not by explicit classloader name.

I've moved this check to canSkipClassLoaderByName, but now this method name doesn't reflect what is happening inside.

should we skip all classloaders with name org.apache.felix.framework.BundleWiringImpl$BundleClassLoader in this short term workaround?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, so those strings are the Bundle-SymbolicName plus the numeric id of the installed bundle allocated by the runtime. Filtering out classloaders this way is fragile and there are potentially other bundles that suffer from the same issue which won't be skipped (since there's nothing particularly special about these atlassian bundles - they are using the same classloader code as anything deployed on the Felix runtime)

TBH I'd probably prefer to drop the logger level in HelperInjector to temporarily avoid this log-spam

@lpriima lpriima changed the title workaround for jira osji modules class loaders workaround for jira OSGi modules class loaders Jan 7, 2021
@lpriima
Copy link
Contributor Author

lpriima commented Feb 1, 2021

workaround not needed after: #2321

@lpriima lpriima closed this Feb 1, 2021
@lpriima lpriima deleted the lp/jiraOSJimodules branch February 1, 2021 06:01
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.

4 participants