-
Notifications
You must be signed in to change notification settings - Fork 791
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 high level Muzzle docs #1270
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our first muzzle doc!!! 🥳
I think muzzle is pervasive enough that this doc deserves to be under docs/contributing
, with a new section/link from CONTRIBUTING.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks!
c905927
to
264109f
Compare
docs/contributing/muzzle.md
Outdated
## Muzzle location | ||
|
||
* `buildSrc` - Muzzle Gradle plugin | ||
* `agent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/muzzle` - Muzzle ByteBuddy plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? In this location there is MuzzleGradlePlugin
but it more seems like ByteBuddy plugin as it implements bytebuddy plugin interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc) @tylerbenson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ByteBuddy plugin is what does the source graph traversal to find all the references for purposes of matching at run time as well as build time in the muzzle
task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a ByteBuddy plugin. The name's a bit misleading and it should be changed in the future -- though to be honest I don't have any great idea right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could rename it to MuzzleByteBuddyPlugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created #1288 for renaming it
docs/contributing/muzzle.md
Outdated
|
||
At runtime the Muzzle checks API compatibility between symbols used by the Agent | ||
and symbols in the application class loader. Before inspecting the class loader | ||
the Muzzle plugin checks symbols defined in `SomeInstrumentation.typeMatcher()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean classLoaderMatcher
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No I meant this
Line 57 in 5d2ae07
return named("com.google.api.client.http.HttpRequest"); |
|
||
## Muzzle location | ||
|
||
* `buildSrc` - Muzzle Gradle plugin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part of muzzle isn't really documented above. This plugin is responsible for downloading the range of libraries from Maven Central and verifying expected compatibility. If a new incompatible version is published, this is what will break the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there also a task to run it explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is what actually runs when you call the muzzle
task.
https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/master/.circleci/config.yml#L132
That can be called at the top level, or for an individual project.
As an optimization, these tasks are only added to gradle if muzzle
is explicitly listed as a task, otherwise it slows down the config stage substantially.
docs/contributing/muzzle.md
Outdated
|
||
At runtime the Muzzle checks API compatibility between symbols used by the Agent | ||
and symbols in the application class loader. Before inspecting the class loader | ||
the Muzzle plugin checks symbols defined in `SomeInstrumentation.typeMatcher()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muzzle actually collects symbols from advice classes -- values of the map returned by transformers()
method. Muzzle by itself calls neither classLoaderMatcher()
nor typeMatcher()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trask mentioned that muzzle first checks API referenced in typeMatcher()
for optimization purposes
docs/contributing/muzzle.md
Outdated
and symbols in the application class loader. Before inspecting the class loader | ||
the Muzzle plugin checks symbols defined in `SomeInstrumentation.typeMatcher()` | ||
as a performance optimization. If the symbols do not match the instrumentation | ||
is not loaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another suggestion 😄
and symbols in the application class loader. Before inspecting the class loader | |
the Muzzle plugin checks symbols defined in `SomeInstrumentation.typeMatcher()` | |
as a performance optimization. If the symbols do not match the instrumentation | |
is not loaded. | |
and symbols in the application class loader. If the symbols do not match the instrumentation is not loaded. | |
Because the muzzle matcher is expensive, it is only performed after a match has been made by the `classLoaderMatcher()` and `typeMatcher()` matchers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. The match for classLoaderMatcher()
and typeMatcher()
is done by ByteBuddy right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's done a few steps before the MuzzleMatcher
runs.
@tylerbenson @iNikem @mateuszrzeszutek @trask @anuraaga thanks for the feedback. The PR is updated with your suggestions. |
@pavolloffay can you run |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
56e5f65
to
e531e97
Compare
Thanks again for this 💯 |
Updates #536