Skip to content

Add Premain-Class attribute #297

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 20, 2023
Merged

Add Premain-Class attribute #297

merged 2 commits into from
Jan 20, 2023

Conversation

nickcaballero
Copy link
Contributor

This PR adds premain method and manifest attribute, supporting the installation of the agent via -javaagent= JVM option.

@pivotal-cla
Copy link

@nickcaballero Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@nickcaballero Thank you for signing the Contributor License Agreement!

@pderop pderop self-assigned this Jan 3, 2023
@pderop pderop added this to the 1.0.7.RELEASE milestone Jan 3, 2023
@pderop pderop added the type/enhancement A general enhancement label Jan 3, 2023
pderop
pderop reviewed Jan 13, 2023
View reviewed changes
Copy link
Contributor

@pderop pderop left a comment

Choose a reason for hiding this comment

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

@nickcaballero ,

Thanks a lot for this PR ! and sorry to only look into it now.

So, I have several comments (some are about problems, while other are about cosmetics)

First, can you please rebase your PR on top of the latest master ?
Also, can you please describe your new feature somewhere in the documentation.

For example, in the Installation section of the quick_start.md, you could add some kind of the following:

If you can't change the application code, you can also activate BlockHound using the -javaagent:<path to BlockHound agent jar> jvm option, something like:

java -javaagent:BlockHound/agent/build/libs/agent.jar -jar my-application.jar

Now, please have a look at the following review, hope you will have time to work on this before we do the next release, let me know

thanks !

pderop
pderop reviewed Jan 13, 2023
View reviewed changes
@pderop pderop added the for/user-attention This issue needs user attention (feedback, rework, etc...) label Jan 13, 2023
@pderop
Copy link
Contributor

pderop commented Jan 17, 2023

I just rebased this PR on top of latest master.

@nickcaballero
Copy link
Contributor Author

@pderop I've updated the PR with your suggestions! Went with the loadIntegrations approach for the builder, which I think is a lot cleaner.

@pderop
Copy link
Contributor

pderop commented Jan 17, 2023

@nickcaballero ,

thanks a lot for updating the PR 😃,

One last small thing, could you just add a note somewhere in the documentation, please check #297 (review) ?

thanks

pderop
pderop approved these changes Jan 18, 2023
View reviewed changes
Copy link
Contributor

@pderop pderop left a comment •
edited
Loading

Choose a reason for hiding this comment

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

thanks for the PR !

+1 from me (I'll update the doc about using the -javaagent in another new PR).

@reactor/core-team , @reactor/netty-team , can someone of you review all this ? thanks.

@pderop pderop requested review from a team January 18, 2023 08:54
chemicL
chemicL reviewed Jan 19, 2023
View reviewed changes
@pderop
Copy link
Contributor

pderop commented Jan 20, 2023 •
edited
Loading

@nickcaballero, thanks for this PR !
@chemicL, thanks for the review !

I'll change the parameter names of the premain method in another PR, let's merge this one for the moment.

@pderop pderop merged commit 4c1dbf9 into reactor:master Jan 20, 2023
@pderop pderop removed the for/user-attention This issue needs user attention (feedback, rework, etc...) label Jan 20, 2023
This was referenced Jan 20, 2023
Closed
Merged
pderop added a commit that referenced this pull request Jan 20, 2023
@pderop
Updated doc for -javaagent JVM option (#323)
5508266 
This PR updates the documentation for the new -javaagent JVM option introduced in #297 , and also applies
a cosmetic feedback that has been made in the odl #297 PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chemicL chemicL chemicL approved these changes

+1 more reviewer

@pderop pderop pderop approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@pderop pderop

Labels
type/enhancement A general enhancement
Projects
None yet
Milestone
1.0.7.RELEASE
Development

Successfully merging this pull request may close these issues.
4 participants
@nickcaballero @pivotal-cla @pderop @chemicL