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

Fix logback appender on android #13234

Merged
merged 1 commit into from
Feb 6, 2025
Merged

Conversation

laurit
Copy link
Contributor

@laurit laurit commented Feb 6, 2025

Hopefully resolves #13230

@laurit laurit requested a review from a team as a code owner February 6, 2025 07:47
@bencehornak
Copy link

Thank you so much for addressing my issue, @laurit! I would like to help to develop a test case to check, what happens, if the ClassLoader throws an exception when trying to load ClassValue.

@bencehornak
Copy link

I wasn't able to find a simple solution to un-define ClassValue. This was my attempt, but it didn't throw the expected ClassNotFoundError on the main branch, so it's not a good test:

class LoggingEventMapperTest {
  // ...

  @Test
  void testClassValueAbsent() throws Exception {
    ClassLoader classLoaderWithoutClassValue = new ClassLoaderWithoutClassValue();

    assertThatNoException()
        .isThrownBy(
            () -> {
              LoggingEventMapper.Builder builder =
                  (LoggingEventMapper.Builder)
                      classLoaderWithoutClassValue
                          .loadClass(LoggingEventMapper.class.getName())
                          .getDeclaredMethod("builder")
                          .invoke(null);
              LoggingEventMapper mapper = builder.build();
              mapper.captureMdcAttributes(Attributes.builder(), new HashMap<>());
            });
  }

  private static class ClassLoaderWithoutClassValue extends ClassLoader {
    @Override
    public Class<?> loadClass(String name) throws ClassNotFoundException {
      // On Android API Level <34 java.lang.ClassValue is absent, see
      // https://developer.android.com/reference/java/lang/ClassValue
      if (name.equals("java.lang.ClassValue")) {
        throw new ClassNotFoundException();
      }
      return super.loadClass(name);
    }
  }
}

It just propagates the classloading of LoggingEventMapper.class to the parent classloader, which has ClassValue in its classpath, so my ClassNotFoundException is never thrown.

Please feel free to proceed with the merging.

@laurit
Copy link
Contributor Author

laurit commented Feb 6, 2025

@bencehornak to make this work you'd need to load LoggingEventMapper in your custom class loader, currently it gets loaded by the parent loader. One way would be https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/jdbc/javaagent/src/test/java/io/opentelemetry/javaagent/instrumentation/jdbc/test/TestClassLoader.java an other typical way is to test in loadClass whether it is the class you wish to override and if it is use one of the resource finders on the parent loader to find the class bytes and call defineClass. Of course in your test you'd have to use reflection all the way to avoid class cast exceptions. Note that this is just FYI you don't need to do anything.

@trask trask merged commit 5f2e5d5 into open-telemetry:main Feb 6, 2025
60 checks passed
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.

Reflection in Logback instrumentation breaks Android support
5 participants