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

Indy plugins #1230

Merged
merged 40 commits into from
Jul 2, 2020
Merged

Indy plugins #1230

merged 40 commits into from
Jul 2, 2020

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Jun 15, 2020

Uses Byte Buddy's new ability to dispatch to advices via INVOKEDYNAMIC.

Excerpt from the Javadoc of IndyBootstrap:

  Bootstrap CL ←──────────────────────────── Agent CL
      ↑ └java.lang.IndyBootstrapDispatcher ─ ↑ ─→ └ {@link IndyBootstrap#bootstrap}
    Ext/Platform CL               ↑          │       ╷
      ↑                           ╷          │       ↓
    System CL                     ╷          │ {@link HelperClassManager.ForIndyPlugin#getOrCreatePluginClassLoader}
      ↑                           ╷          │       ╷
    Common               linking of CallSite │       ╷
    ↑    ↑             (on first invocation) │    creates
WebApp1  WebApp2                  ╷          │       ╷
         ↑ - InstrumentedClass    ╷          │       ╷
         │                ╷       ╷          │       ╷
         │                INVOKEDYNAMIC      │       ↓
         └────────────────┼──────────────────Plugin CL
                          └╶╶╶╶╶╶╶╶╶╶╶╶╶╶╶╶→ ├ AdviceClass
                                             └ AdviceHelper
Legend:
 ╶╶→ method calls
 ──→ class loader parent/child relationships

First step of #1085
Supersedes #1090

Refactors the JDBC and Servlet plugin to be indy plugins. Prepares some more plugins for easy migration to indy plugins.

felixbarny added 18 commits June 2, 2020 13:47
- Add AssignTo annotation to assign to different targets by returning an Object[]
- Add GlobalState annotation
- Create ThreadLocalRegistry to easily construct global thread locals
- Patch class file version to at least 51 (Java 7) in order to be able to insert
  invokedynamic instructions
- Load the whole package of the Advice class from a dedicated plugin classloader
  that's a child of the classloader of the instrumented class
- Depend on byte-buddy-dep and manually shade asm so that we can use asm-commons
- Rename helper class loader to plugin class loader
- Add GlobalVariables registry
@apmmachine
Copy link
Contributor

apmmachine commented Jun 15, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #1230 updated]

  • Start Time: 2020-07-01T10:05:09.990+0000

  • Duration: 43 min 29 sec

Test stats 🧪

Test Results
Failed 0
Passed 556
Skipped 11
Total 567

@SylvainJuge
Copy link
Member

As said during the meeting we had today:

  • we might slightly improve @AssignTo* annotations by using nested annotations in a similar way as @Advice.Return or @Advice.Local with that we could have : @AssignTo.ReturnValue, @AssignTo.Parameter and so on.
  • Finding a way to use named keys to bind to method parameters (instead of confusing index and value), could also be a good improvement. For example annotating an advice parameter with @AssignTo.Key("value") could allow to then refer to it by name (rather than index) in @AdviceTo annotation.

- Fix matcher for ForkJoinPool
- Add support for parallel streams
- Simplify testing of bootstrap instrumentations
- Remove java.* from the default exclusions
- Disallow agent types in advice signature
- Enable previously disabled async Dubbo tests
@felixbarny felixbarny mentioned this pull request Jun 19, 2020
9 tasks
allow agent classes to be instrumented
- Don't start agent on early J9 versions that crash on indy bootstrap
- Fix location strategy order to avoid ClassNotFoundExceptions on Payara
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Epic PR!! That's really a whole new agent technology!

First review chunk - looks great so far, didn't find anything major, a few minor comments/suggestions/questions.

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Chunk 2, a smaller one today...

@felixbarny felixbarny mentioned this pull request Jun 30, 2020
20 tasks
@felixbarny felixbarny mentioned this pull request Jul 2, 2020
20 tasks
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

LGTM.
Did we get to a threshold of doing more non-inlined than inlined? If so, can and should we configure BB to not-inline by default and explicitly define only when we want to inline?

Comment on lines +93 to +97
assertThat(Stream.of("foo", "bar", "baz")
.parallel()
.<AbstractSpan<?>>map(s -> tracer.getActive())
.distinct())
.containsExactly(transaction);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even more explicit is:

Suggested change
assertThat(Stream.of("foo", "bar", "baz")
.parallel()
.<AbstractSpan<?>>map(s -> tracer.getActive())
.distinct())
.containsExactly(transaction);
assertThat(Stream.of("foo", "bar", "baz")
.parallel()
.<AbstractSpan<?>>map(s -> tracer.getActive()))
.containsExactly(transaction, transaction, transaction);

Copy link
Member Author

Choose a reason for hiding this comment

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

True, but I don't want to execute the tests again because of that.

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