Runtime attachment additions

[jeanbisutti]

Description:

Runtime attachment additions

• Add tests
• Add log when agent is disabled with otel.javaagent.enabled property
• Add log when agent is disabled with OTEL_JAVAAGENT_ENABLED environment variable
• Do not try to attach if agent is already attached
• Do not try to attach if attachment is requested from the main thread
• Do not try to attach if attachment is requested from the main method
• Add log if an unexpected issue has happened during attachment
• Add javadoc
• Add information to the Readme

[trask]

thx!

[trask]

do we need to check the env var and system property? it may be ok for the runtime attach component to try to attach and the javaagent will disable itself if these are set

[jeanbisutti]

I understand your point of view @trask . It could make sense. I would prefer not to try to attach, in such a case, to stop the things sooner. It's the current behavior. What do you think @iNikem?

[trask]

ah, I hadn't noticed this was the current behavior, let's keep it then, thx

[trask]

what do you think about adding one more check, that the call stack only has main in it, i.e. that it was really called directly from the initial main method?

[jeanbisutti]

I think it's a good idea. I will do it. I could even check that the first element of the call stack contains main.

[jeanbisutti]

Implemented. The new code be tested with the following class:

public class MainClass {

  public static void main(String[] args) {
    System.out.println("isMainMethod = " + isMainMethod());
  }

  static boolean isMainMethod() {
    StackTraceElement bottomOfStack = findBottomOfStack(Thread.currentThread());
    String methodName = bottomOfStack.getMethodName();
    return "main".equals(methodName);
  }

  private static StackTraceElement findBottomOfStack(Thread thread) {
    StackTraceElement[] stackTrace = thread.getStackTrace();
    return  stackTrace[stackTrace.length - 1];
  }
}

[trask]

I think typically the main thread will always be rooted at the main() method(?), I was thinking of a more restrictive chck that attach() was called directly from main()

[trask]

I'm going to merge this for the 1.15.0 release, and we can discuss / follow-up on this point later

[trask]

can you move this version to dependency management section so we don't miss updating it?

[jeanbisutti]

Done

[trask]

thx @jeanbisutti!

[mateuszrzeszutek]

I might be a little bit late here, but... 😄

Should we use JUL here? This class is supposed to be the very first thing called in main, right? JUL won't be set up this early, and these calls certainly won't be redirected to agent's PatchLogger. Wouldn't it be safer to use stderr here?

[jeanbisutti]

Should we use JUL here? This class is supposed to be the very first thing called in main, right? JUL won't be set up this early, and these calls certainly won't be redirected to agent's PatchLogger. Wouldn't it be safer to use stderr here?

Probably!

What about the logging added by a user? For example, if a user has a Spring Boot application and uses a logging API (jul, Log4j, logback, ...) in the class annotated with @SpringBootApplication, it would also have a problem? The problem would only be about JUL?

...
import java.util.logging.Logger;
...
private static final Logger logger = Logger.getLogger(SpringBootApp.class.getName());
...
@SpringBootApplication
public class SpringBootApp {
    public static void main(String[] args) {
        RuntimeAttach.attachJavaagentToCurrentJVM();
        SpringApplication.run(SpringBootApp.class, args);
    }
}

The user should initialize the logger after the runtime attachment?

...
import java.util.logging.Logger;
...
private static Logger logger;
...
@SpringBootApplication
public class SpringBootApp {
    public static void main(String[] args) {
        RuntimeAttach.attachJavaagentToCurrentJVM();
        logger = Logger.getLogger(SpringBootApp.class.getName());
        SpringApplication.run(SpringBootApp.class, args);
    }
}

@mateuszrzeszutek @trask What do you think?

[trask]

thx, I agree with changing the logging in the runtime attach code itself to use stderr so that we don't trigger loading of j.u.l too early, e.g. in case the user is using jboss-logging (maybe https://www.wildfly.org/news/2020/06/18/Bootable-jar-Wildfly-20/?)

I don't think we need to add any restrictions on the user's logging setup. If whatever they were doing before they added the "runtime attach snippet" worked, it should still work after they add the snippet.