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

Add -XX:+EnableDynamicAgentLoading for JEP 451 #25268

Draft
wants to merge 1 commit into
base: 8.0
Choose a base branch
from

Conversation

dmiya3
Copy link
Contributor

@dmiya3 dmiya3 commented Dec 5, 2024

Currently, starting GlassFish on JDK 21 and then executing asadmin enable-monitoring prints the following message in the server log:

[2024-12-02T19:08:59.248054+09:00] [GF 8.0.0-M7] [SEVERE] [] [jakarta.enterprise.logging.stderr] [tid: _ThreadID=90 _ThreadName=Attach Listener] [levelValue: 1000] [[
  WARNING: A Java agent has been loaded dynamically (/opt/glassfish8-org/glassfish/lib/monitor/flashlight-agent.jar)
WARNING: If a serviceability tool is in use, please run with -XX:+EnableDynamicAgentLoading to hide this warning
WARNING: If a serviceability tool is not in use, please run with -Djdk.instrument.traceUsage for more information
WARNING: Dynamic loading of agents will be disallowed by default in a future release]]

Signed-off-by: dmiya3 [email protected]

@dmiya3 dmiya3 marked this pull request as draft December 6, 2024 02:24
@dmatej
Copy link
Contributor

dmatej commented Dec 6, 2024

I was thinking why the PR doesn't lead to master, now I know ... the JEP 451 is for Java 21, so the option would not exist in 11-17.
Perhaps we could add it dynamically depending on Java version, I have found there is a place for that: JvmOptions class; I already touched this class for GlassFish 7.1.0 in #25183 and added the addMissing method. The logic is "if user did not configure the option, we set it this way".
What do you think about it? I am not sending the direct link as I am rebasing that branch and at some point the link would break.

        filter();
        addMissing();
        setOsgiPort();

@OndroMih
Copy link
Contributor

OndroMih commented Dec 6, 2024

My view:

  • adding default options by the JVMOptions class should be preferred over adding it to the default domain.xml, because it’s then automatically applied also when upgrading a domain config from older GlassFish
  • JVMOptions should add an option only when the JDK version supports it. I don’t know how it finds out - probably by calling java -version first, but that slows startup of the server
  • It should always be possible to disable a specific option. E.g. if JVM options in domain.xml already contains -XX:-EnableDynamicAgentLoading, then JVMOptions class should add -XX:+EnableDynamicAgentLoading

@dmatej
Copy link
Contributor

dmatej commented Dec 6, 2024

@OndroMih The JVMOptions class processes values from domain.xml; The Java version can be retrieved from the Runtime.version() call.

@OndroMih
Copy link
Contributor

OndroMih commented Dec 6, 2024

Getting the version from Runtime.version() would not be correct. It would be the version of Java used to run the asadmin command, which might be different from the version used to run the server. Remember that it’s possible to specify path the the JVM in domain.xml. In that case, the logic should check the version of Java specified in domain.xml. If it’s not set, then yes, it could just use Runtime.version(), assuming that the server is started with the same JDK.

@hs536
Copy link
Contributor

hs536 commented Dec 10, 2024

Adding this JVM option to GlassFish also hides warnings intended for the user application, so I'm hesitant to merge this change... Is it not straightforward to modify the monitor func(?) to avoid using DynamicAgentLoading?

@dmatej
Copy link
Contributor

dmatej commented Dec 11, 2024

Remember that it’s possible to specify path the the JVM in domain.xml.

    private void setJavaExecutable() throws GFLauncherException {
        // first choice is from domain.xml
        if (setJavaExecutableIfValid(domainXMLjavaConfig.getJavaHome())) {
            return;
        }

        // second choice is from asenv
        if (!setJavaExecutableIfValid(asenvProps.get(JAVA_ROOT_PROPERTY))) {
            throw new GFLauncherException("nojvm");
        }
    }

You are right, then the only option is to execute the java command and get the Java version from that, or execute some generated java class using ProcessBuilder to get directly the value we need (the java -version output is not standardized). And we can also avoid it if the java command resolves to the same file as we are currently running.

Another option is Hiroki's idea - does Flashlight really need the dynamic loading? Maybe then it stops being "flash", I don't know. Didn't Adam Bien use it years ago? I have no experience with this feature.

@OndroMih
Copy link
Contributor

I think we can include flashlight agent directly on the command line immeiately, and not load it lazily at runtime. The only thing that the agent does is it stores the Instrumentation context, it seems. The context is then used by reflection if the agent is loaded. So loading the agent everytime at startup is almost no overhead.

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