Skip to content

Conversation

Copilot
Copy link

@Copilot Copilot AI commented Aug 18, 2025

Summary

Fixes a class loader mismatch issue in OtlpHttpMetricExporter.toBuilder() where the HttpExporterBuilder.copy() method was not preserving the componentLoader field, causing new instances to use default values instead of the original configuration.

Problem

When extending OtlpMetricExporterProvider to dynamically inject authentication headers, calling OtlpHttpMetricExporter.toBuilder() caused a class loader mismatch. The issue occurred because:

  1. The original exporter was configured with a custom ComponentLoader (e.g., MultipleParentClassLoader in agent scenarios)
  2. toBuilder() internally calls HttpExporterBuilder.copy()
  3. The copy() method was not preserving the componentLoader field
  4. The new builder instance would use the default AgentClassLoader instead
  5. This caused the JDK sender to become unavailable, breaking metric export

Example usage that failed before this fix:

public class AuthenticatedOtlpMetricExporterProvider extends OtlpMetricExporterProvider {
    @Override
    public MetricExporter createExporter(ConfigProperties config) {
        ClientCredentialsAuthenticator auth = ...;
        MetricExporter metricExporter = super.createExporter(config);

        if (metricExporter instanceof OtlpHttpMetricExporter) {
            // This would fail with class loader mismatch
            OtlpHttpMetricExporter.Builder otlpHttpBuilder =
                ((OtlpHttpMetricExporter) metricExporter).toBuilder();
            otlpHttpBuilder.setHeaders(auth::getHeaders);
            return otlpHttpBuilder.build();
        }
        // ...
    }
}

Solution

Fixed the HttpExporterBuilder.copy() method to preserve the componentLoader field:

copy.componentLoader = componentLoader;

Note: The executorService field is intentionally not copied to allow each exporter instance to manage its own executor lifecycle.

Testing

Added comprehensive tests to verify the fix:

  1. Unit tests in HttpExporterBuilderTest to ensure copy() preserves the componentLoader field
  2. Integration test in OtlpHttpMetricExporterBuilderTest to verify toBuilder() works correctly with custom component loaders
  3. Regression tests to ensure existing functionality remains intact

The fix ensures that when toBuilder() is called on an OTLP HTTP exporter, the resulting builder maintains the same component loader configuration, preventing class loader mismatches in agent environments.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI changed the title [WIP] **Describe the bug** When extending OtlpMetricExporterProvider to dynamically inject authentication headers, calling OtlpHttpMetricExporter.toBuilder() causes a classloader mismatch. The HttpExporterBuilder.copy() method used internally does no... Fix HttpExporterBuilder.copy() to preserve componentLoader and executorService fields Aug 18, 2025
@Copilot Copilot AI requested a review from trask August 18, 2025 21:03
Copilot finished work on behalf of trask August 18, 2025 21:03
Copy link
Owner

@trask trask left a comment

Choose a reason for hiding this comment

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

@copilot Don't copy executor service

@trask
Copy link
Owner

trask commented Aug 18, 2025

@copilot don't copy over the executor service

Copy link
Author

Copilot AI commented Aug 18, 2025

@copilot Don't copy executor service

Done. Removed the executorService copying from HttpExporterBuilder.copy() method and its associated test. Only the componentLoader field is now preserved to fix the class loader mismatch issue. See commit 26f9e7d.

@copilot don't copy over the executor service

Done. Removed the executorService copying from HttpExporterBuilder.copy() method and its associated test. Only the componentLoader field is now preserved to fix the class loader mismatch issue. See commit 26f9e7d.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dl.google.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -XX:MaxMetaspaceSize=512m -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/gradle-daemon-main-8.14.3.jar -javaagent:/home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/agents/gradle-instrumentation-agent-8.14.3.jar org.gradle.launcher.daemon.bootstrap.GradleDaemon 8.14.3 (dns block)
  • scans-in.gradle.com
    • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -XX:MaxMetaspaceSize=512m -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/gradle-daemon-main-8.14.3.jar -javaagent:/home/REDACTED/.gradle/wrapper/dists/gradle-8.14.3-bin/cv11ve7ro1n3o1j4so8xd9n66/gradle-8.14.3/lib/agents/gradle-instrumentation-agent-8.14.3.jar org.gradle.launcher.daemon.bootstrap.GradleDaemon 8.14.3 (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@Copilot Copilot AI changed the title Fix HttpExporterBuilder.copy() to preserve componentLoader and executorService fields Fix HttpExporterBuilder.copy() to preserve componentLoader field Aug 18, 2025
@Copilot Copilot AI requested a review from trask August 18, 2025 21:44
Copilot finished work on behalf of trask August 18, 2025 21:44
Comment on lines +94 to +104

@Test
void copy_preservesComponentLoader() {
ComponentLoader customComponentLoader =
ComponentLoader.forClassLoader(new URLClassLoader(new URL[0], null));
builder.setComponentLoader(customComponentLoader);

HttpExporterBuilder<Marshaler> copiedBuilder = builder.copy();

assertThat(copiedBuilder).extracting("componentLoader").isEqualTo(customComponentLoader);
}
Copy link
Owner

Choose a reason for hiding this comment

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

if there aren't other similar tests for other HttpExporter fields, then remove this

@@ -237,4 +238,27 @@ void verifyToBuilderPreservesSettings() {
original.close();
copy.close();
}

@Test
void toBuilderPreservesComponentLoader() {
Copy link
Owner

Choose a reason for hiding this comment

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

why not merge this into above verifyToBuilderPreservesSettings?

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.

2 participants