Extract javaagent-extension-api from tooling & spi #2879

mateuszrzeszutek · 2021-04-27T15:13:30Z

Implementation of (a part of) the module split discussed here: #2779

Sorry for huge PR, but I couldn't really split it further - fortunately most of it are renames and import changes.

All classes used to implement a javaagent instrumentation/extension were extracted to the javaagent-extension-api module:

All javaagent instrumentations are now using this module and do not depend on javaagent-tooling directly; everything that was left in javaagent-tooling is an internal javaagent component that's not supposed to be exposed or used by instrumentation developers.

I've introduced the AgentExtension interface which InstrumentationModule now implements; it also replaces ByteBuddyAgentCustomizer (which kind of resolves #1626). The AgentExtension serves as a base for any agent modification/enrichment, including instrumentation.

All interesting changes are in javaagent-tooling and javaagent-extension-api, almost everything else is just import renames.

mateuszrzeszutek · 2021-04-27T15:35:55Z

Ouch, this PR has so many changed files that it's almost impossible to review in GH... I'll have to figure out how to split it (at least split it to several commits) after all.

trask · 2021-04-27T16:57:42Z

Ouch, this PR has so many changed files that it's almost impossible to review in GH... I'll have to figure out how to split it (at least split it to several commits) after all.

It's rendering for me

trask · 2021-04-27T17:02:09Z

Ouch, this PR has so many changed files that it's almost impossible to review in GH... I'll have to figure out how to split it (at least split it to several commits) after all.

It's rendering for me

I realized that's not what you said, ya, it's a lot of files, how about just a separate commit in this PR for the stuff under javaagent-tooling and javaagent-extension-api?

mateuszrzeszutek · 2021-04-27T17:49:14Z

I realized that's not what you said, ya, it's a lot of files, how about just a separate commit in this PR for the stuff under javaagent-tooling and javaagent-extension-api?

I'll try to separate all renames/imports into a separate commit tomorrow, that should make reviewing the important stuff much easier.

mateuszrzeszutek · 2021-04-28T18:15:19Z

Okay, I've managed to split this PR into 2 commits:

the first one contains (mostly) extracting the javaagent-extension-api module, almost no renames/moving between packages;
the second one is moving InstrumentationModule, muzzle and type matchers and is pretty much just renames.

trask

thanks for tackling this!!

trask · 2021-04-29T20:57:08Z

javaagent-extension-api/javaagent-extension-api.gradle

+
+dependencies {
+  api deps.opentelemetrySdk
+  api deps.opentelemetrySdkMetrics


even though it's technically an api dependency, we could consider making it implementation and marking extension-api methods that use it as @Unstable (had similar discussion with @anuraaga last week about instrumentation-api's dependency on metrics)

would just be a compromise if we want to mark the extension-api stable before the metrics api is stable

trask · 2021-04-29T21:55:37Z

...-extension-api/src/main/java/io/opentelemetry/javaagent/extension/AgentExtensionTooling.java

+ * This interface contains methods for accessing or creating internal javaagent components that can
+ * be used to implement an {@link AgentExtension}.
+ */
+public interface AgentExtensionTooling {


I was having a hard time understanding the need for this as a public API surface, but I think I understand now:

it's needed to break the dependency from javaagent-extension-api on javaagent-tooling

just wanted to check/confirm my understanding...

but why is this public?

it's needed to break the dependency from javaagent-extension-api on javaagent-tooling

just wanted to check/confirm my understanding...

Yep, it's exactly that. This interface contains everything that we need from tooling/internals in instrumentations.

but why is this public?

The whole interface? It's a parameter of one AgentExtension method which itself is public, and I suppose it does contain some useful things (like access to classloaders) for extension developers.

trask · 2021-04-29T21:59:22Z

...agent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/ConstantAdjuster.java

+ *
+ * @see ConstantAdjuster The ASM visitor that does the actual work.
+ */
+public final class ConstantAdjuster implements AgentBuilder.Transformer {


could be package-protected if moved to same package with InstrumentationModule

iNikem · 2021-05-03T15:28:10Z

I took only a quick look so far and my main concern is the number of public classes. E.g. do we really expect now that users will provide their own implementations of InstrumentationContextProvider? I understand that this is needed for a split between javaagent-extension-api and javaagent-tooling as @trask noted. But extension module uses them, I think, only inside io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule#extend. Should the body of this method actually live in tooling module?

Also, how exactly is this interface specific for instrumentation context?

mateuszrzeszutek · 2021-05-04T07:37:15Z

E.g. do we really expect now that users will provide their own implementations of InstrumentationContextProvider?

No, this interface is supposed to hide the actual implementation, not be implemented.

But extension module uses them, I think, only inside io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule#extend. Should the body of this method actually live in tooling module?

That's an interesting idea - we would certainly be able to hide some more things this way. I'll think about how to do it nicely.

Also, how exactly is this interface specific for instrumentation context?

With method names like additionalInstrumentation() - probably not at all? 😂
More reasons to hide it.

mateuszrzeszutek · 2021-05-04T16:46:15Z

io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule#extend - should the body of this method actually live in tooling module?

Done! The AgentExtensionTooling interface got considerably smaller, now I'm starting to think that maybe I could get rid of it completely... Please review the current implementation of InstrumentationModule#extend() and let me know what you think - I could use a similar trick to hide those muzzle deps.

iNikem · 2021-05-05T07:31:46Z

@mateuszrzeszutek What if InstrumentationModule should NOT be a AgentExtension? What if there should instead be InstrumentationLoader implements AgentExtension? And its extend method loads all InstrumentationModule, which provide all necessary information to apply each module to AgentBuilder. InstrumentationLoader will live javaagent-tooling module then and will take AgentExtensionTooling with it.

mateuszrzeszutek · 2021-05-06T09:46:22Z

What if there should instead be InstrumentationLoader implements AgentExtension? And its extend method loads all InstrumentationModule, which provide all necessary information to apply each module to AgentBuilder. InstrumentationLoader will live javaagent-tooling module then and will take AgentExtensionTooling with it.

Hmm, interesting. If we move the ReferenceMatcher to the tooling module (and it's possible if we change the return type of getMuzzleReferenceMatcher() to Reference[]) then we could get rid of the AgentExtensionTooling class.
I kind of liked the fact that we had one SPI less, but that's not such a big deal after all - and we can simplify javaagent-extension-api so that certainly makes it worth it.

Do you mind if I implement it in another PR? This one is both hard to maintain (conflicts...) and hard to review...

This reverts commit 7aa517c29e7ddfb02a69121a3693514d4a69aa5a.

…ternal

mateuszrzeszutek · 2021-05-06T14:02:17Z

Okay, I moved all muzzle classes except Reference back to tooling and got rid of the AgentExtensionTooling interface. InstrumentationModule still implements AgentExtension, but now the change that @iNikem proposed should be really easy to implement.

mateuszrzeszutek · 2021-05-06T14:03:05Z

And now the package structure looks like that:

trask · 2021-05-06T18:38:32Z

I moved all muzzle classes except Reference back to tooling and got rid of the AgentExtensionTooling interface

👍👍👍

Do you mind if I implement it in another PR? This one is both hard to maintain (conflicts...) and hard to review...

Yes, let's merge this! @iNikem

iNikem · 2021-05-07T06:12:51Z

I moved all muzzle classes except Reference back to tooling and got rid of the AgentExtensionTooling interface

👍👍👍

Do you mind if I implement it in another PR? This one is both hard to maintain (conflicts...) and hard to review...

Yes, let's merge this! @iNikem

I will trust you on that, @trask

mateuszrzeszutek requested review from anuraaga, iNikem, jkwatson, laurit, pavolloffay, trask and tylerbenson as code owners April 27, 2021 15:13

mateuszrzeszutek force-pushed the javaagent-extension-api branch 2 times, most recently from eea280c to 6c8d66b Compare April 28, 2021 18:12

trask approved these changes Apr 29, 2021

View reviewed changes

mateuszrzeszutek force-pushed the javaagent-extension-api branch from 8086767 to b7bf82c Compare May 4, 2021 07:40

mateuszrzeszutek force-pushed the javaagent-extension-api branch 2 times, most recently from 28a1259 to 0ea9e95 Compare May 6, 2021 10:32

Mateusz Rzeszutek added 7 commits May 6, 2021 16:00

Extract javaagent-extension-api from tooling & spi

88b087e

Move classes to io.opentelemetry.javaagent.extension package

d4b7399

This reverts commit 7aa517c29e7ddfb02a69121a3693514d4a69aa5a.

fixup! Extract javaagent-extension-api from tooling & spi

6096e36

Code review comments

3340b17

Move InstrumentationModule#extend() implementation back to tooling/in…

018ea48

…ternal

spotless

6b0f327

Trim javaagent-extension-api

0a954b5

mateuszrzeszutek force-pushed the javaagent-extension-api branch from 0ea9e95 to 0a954b5 Compare May 6, 2021 14:00

Mateusz Rzeszutek added 6 commits May 6, 2021 16:06

Fix tests

f1cc51c

Fix compilation failure

44d0024

Fix javadocs

3839d7d

Fix :javaagent:test

7889aac

Remove TODO

dfe7f99

Fix wrong import statement

6472aeb

anuraaga approved these changes May 6, 2021

View reviewed changes

trask merged commit 9c7fae3 into open-telemetry:main May 7, 2021

mateuszrzeszutek deleted the javaagent-extension-api branch May 7, 2021 06:48

mateuszrzeszutek mentioned this pull request May 12, 2021

Load InstrumentationModules using InstrumentationLoader #2971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract javaagent-extension-api from tooling & spi #2879

Extract javaagent-extension-api from tooling & spi #2879

mateuszrzeszutek commented Apr 27, 2021 •

edited

Loading

mateuszrzeszutek commented Apr 27, 2021

trask commented Apr 27, 2021

trask commented Apr 27, 2021

mateuszrzeszutek commented Apr 27, 2021

mateuszrzeszutek commented Apr 28, 2021

trask left a comment

trask Apr 29, 2021

trask Apr 29, 2021

iNikem May 3, 2021

mateuszrzeszutek May 4, 2021

trask Apr 29, 2021

iNikem commented May 3, 2021

mateuszrzeszutek commented May 4, 2021 •

edited

Loading

mateuszrzeszutek commented May 4, 2021

iNikem commented May 5, 2021

mateuszrzeszutek commented May 6, 2021

mateuszrzeszutek commented May 6, 2021

mateuszrzeszutek commented May 6, 2021

trask commented May 6, 2021

iNikem commented May 7, 2021

Extract javaagent-extension-api from tooling & spi #2879

Extract javaagent-extension-api from tooling & spi #2879

Conversation

mateuszrzeszutek commented Apr 27, 2021 • edited Loading

mateuszrzeszutek commented Apr 27, 2021

trask commented Apr 27, 2021

trask commented Apr 27, 2021

mateuszrzeszutek commented Apr 27, 2021

mateuszrzeszutek commented Apr 28, 2021

trask left a comment

Choose a reason for hiding this comment

trask Apr 29, 2021

Choose a reason for hiding this comment

trask Apr 29, 2021

Choose a reason for hiding this comment

iNikem May 3, 2021

Choose a reason for hiding this comment

mateuszrzeszutek May 4, 2021

Choose a reason for hiding this comment

trask Apr 29, 2021

Choose a reason for hiding this comment

iNikem commented May 3, 2021

mateuszrzeszutek commented May 4, 2021 • edited Loading

mateuszrzeszutek commented May 4, 2021

iNikem commented May 5, 2021

mateuszrzeszutek commented May 6, 2021

mateuszrzeszutek commented May 6, 2021

mateuszrzeszutek commented May 6, 2021

trask commented May 6, 2021

iNikem commented May 7, 2021

mateuszrzeszutek commented Apr 27, 2021 •

edited

Loading

mateuszrzeszutek commented May 4, 2021 •

edited

Loading