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

Document muzzle #536

Closed
iNikem opened this issue Jun 16, 2020 · 13 comments · Fixed by #2793
Closed

Document muzzle #536

iNikem opened this issue Jun 16, 2020 · 13 comments · Fixed by #2793
Labels
contributor experience Something that we can do to improve the experience of contributors (especially new contributors) documentation Improvements or additions to documentation

Comments

@iNikem
Copy link
Contributor

iNikem commented Jun 16, 2020

The whole story around muzzle is extremely confusing, but one encounters it almost everywhere in this repo. It has 0 documentation and huge number of moving parts:

  • Muzzle plugin in buildSrc folder
  • Two muzzle plugins in auto-tooling module
  • Muzzle task and its configuration
  • Muzzle task's logs that one has to read and understand when muzzle task fails
  • What does it mean "instrumentation was muzzled"??
  • What is muzzle's relationship with different class loaders? I sense there is one, but I am not sure.
  • What is MuzzleMatcher and how is it connected to muzzle task and muzzle plugins?
  • What does the javadoc on io.opentelemetry.auto.tooling.Instrumenter.Default#getInstrumentationMuzzle actually mean? And why do we generate our own code with "compile-time bytecode transformations"?
  • Muzzle plugin does some extremely convoluted things with Gradle. Such as arbitrarily reaching to other projects. This makes it impossible to use Gradle's configuration on demand. That could significantly increase build time in some cases.
  • Why MuzzleVisitor uses ASM and not ByteBuddy?
@anuraaga
Copy link
Contributor

+100 - getting a presentation from @trask helped me a lot but indeed there's so much muzzle around I do get re-lost.

Want to give credit for the help in working around the issues from reviewers but it's naturally not as scalable as us having a proper guide to muzzle. Based on the name, I assume it's originally datadog and I hope they can help us with documenting this to improve the contributor experience.

@trask trask added the documentation Improvements or additions to documentation label Jun 16, 2020
@trask trask self-assigned this Jun 16, 2020
@trask
Copy link
Member

trask commented Jun 16, 2020

I will take a first stab at this, and call in @tylerbenson for backup as needed

@trask trask added the contributor experience Something that we can do to improve the experience of contributors (especially new contributors) label Jul 10, 2020
@trask
Copy link
Member

trask commented Jul 13, 2020

See #654 for another muzzle thing to investigate and doc

@trask
Copy link
Member

trask commented Jul 15, 2020

Another interesting thing to doc: #696

@trask
Copy link
Member

trask commented Jul 17, 2020

Another good thing to doc: #701 (comment)

@trask
Copy link
Member

trask commented Jul 17, 2020

And another: #694 (comment)

@trask
Copy link
Member

trask commented Aug 9, 2020

Another quirk: updates to muzzle java sources in auto-tooling don't seem to take effect when running

./gradlew :instrumentation:<module>:muzzle

without adding --rerun-tasks

@anuraaga
Copy link
Contributor

anuraaga commented Sep 3, 2020

@trask #1121 (comment) :)

@trask
Copy link
Member

trask commented Sep 4, 2020

@trask #1121 (comment) :)

e.g. recommend instrumentation authors not to use lambdas

@trask
Copy link
Member

trask commented Sep 23, 2020

More

  • Doc assertInverse
  • Does muzzle fail if an Instrumentation doesn't match anything? E.g. seems it doesn't fail if it doesn't match, see running muzzle against elasticsearch 7 doesn't fail Elasticsearch6TransportClientInstrumentation even though Action class doesn't exist anymore

@trask
Copy link
Member

trask commented Sep 27, 2020

Doc printReferences task, e.g.

./gradlew :instrumentation:google-http-client-1.19:printReferences

@trask
Copy link
Member

trask commented Sep 27, 2020

Youtube clip from SIG meeting describing muzzle, maybe worth referencing(?)

https://www.youtube.com/watch?v=2iL91CwfxCo&feature=youtu.be&t=2682

@mateuszrzeszutek
Copy link
Member

I've compiled a list of questions that have not been answered yet in our muzzle docs:

  • Muzzle task's logs that one has to read and understand when muzzle task fails
    • Should we work on the readability of those logs? They seemed okay to me, but maybe we can improve them somehow.
  • Muzzle plugin does some extremely convoluted things with Gradle. Such as arbitrarily reaching to other projects. This makes it impossible to use Gradle's configuration on demand. That could significantly increase build time in some cases.
  • Why MuzzleVisitor uses ASM and not ByteBuddy?
  • do not use static fields in InstrumentationModule/TypeInstrumentation (and probably advice classes too)
  • Does muzzle fail if an Instrumentation doesn't match anything? E.g. seems it doesn't fail if it doesn't match, see running muzzle against elasticsearch 7 doesn't fail Elasticsearch6TransportClientInstrumentation even though Action class doesn't exist anymore
    • I assume that this concerns the typeMatcher()/transformers() methods, right?

I've skipped questions that no longer apply or will be solved by #1714 or that have been answered. Is there anything else that we should add here, any other questions?
I probably have a different idea about what's obvious and does not need explaining due to working with muzzle a lot, so don't hesitate to ask even obvious things, I'll add them all to docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor experience Something that we can do to improve the experience of contributors (especially new contributors) documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants