Skip to content

Conversation

@SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Sep 18, 2020

What does this PR do?

JDBC instrumentation fails on some environments when active.
This appears to be a side-effect of migration to new plugin architecture introduced in version 1.18.0.
We haven't been able to reproduce this directly, but changes have been confirmed to work where it used to happen.

Checklist

  • This is a bugfix

@ghost
Copy link

ghost commented Sep 18, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1409 updated]

  • Start Time: 2020-09-28T19:35:46.075+0000

  • Duration: 46 min 21 sec

Test stats 🧪

Test Results
Failed 0
Passed 1562
Skipped 12
Total 1574

@SylvainJuge SylvainJuge marked this pull request as ready for review September 28, 2020 14:09
@SylvainJuge SylvainJuge changed the title WIP try to lazy-load jdbc helper lazy-load jdbc helper Sep 28, 2020
}

protected synchronized static JdbcHelper getJdbcHelper() {
// lazily initialize helper to prevent trying to load classes in java.sql package with the bootstrap classloader
Copy link
Member

Choose a reason for hiding this comment

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

Is that an indication that we should more strictly separate advices from element matches?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say yes, separation would be better. In the same category, there is getAdviceClass() method that returns the advice class loaded from agent/bootstrap classloader when in practice we only use class name and then load it in child-first plugin classloader. We already discussed that a bit with @eyalkoren last week.

@SylvainJuge SylvainJuge merged commit 2d2057f into elastic:master Sep 29, 2020
@SylvainJuge SylvainJuge deleted the try-lazy-load-jdbchelper branch September 29, 2020 14:42
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.

3 participants