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

Make spring boot service name detector handle BOOT-INF/classes #8101

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,18 @@ private String loadFromClasspath(String filename, Function<InputStream, String>

// Exists for testing
static class SystemHelper {
private final ClassLoader classLoader;
private final boolean addBootInfPrefix;

SystemHelper() {
ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
Copy link
Member

Choose a reason for hiding this comment

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

Is getting the context CL necessary? This class runs in SDK setup time; in the javaagent it shouldn't have any context CL set (I've removed the setContextClassLoader calls a while ago)

Copy link
Contributor Author

@laurit laurit Mar 22, 2023

Choose a reason for hiding this comment

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

There isn't anything that says this has to be used only with javaagent. I think the context class loader defaults to system when it is not set (not set to null or some other class loader by the user).

classLoader =
contextClassLoader != null ? contextClassLoader : ClassLoader.getSystemClassLoader();
addBootInfPrefix = classLoader.getResource("BOOT-INF/classes/") != null;
if (addBootInfPrefix) {
logger.log(Level.FINER, "Detected presence of BOOT-INF/classes/");
}
}

String getenv(String name) {
return System.getenv(name);
Expand All @@ -275,7 +287,8 @@ String getProperty(String key) {
}

InputStream openClasspathResource(String filename) {
return ClassLoader.getSystemClassLoader().getResourceAsStream(filename);
String path = addBootInfPrefix ? "BOOT-INF/classes/" + filename : filename;
return classLoader.getResourceAsStream(path);
}

InputStream openFile(String filename) throws Exception {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import io.opentelemetry.api.common.Attributes;
import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.resources.Resource;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.URL;
import java.nio.file.Files;
Expand Down Expand Up @@ -45,7 +46,7 @@ void findByEnvVar() {

@Test
void classpathApplicationProperties() {
when(system.openClasspathResource(PROPS)).thenCallRealMethod();
when(system.openClasspathResource(PROPS)).thenReturn(openClasspathResource(PROPS));
SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector(system);
Resource result = guesser.createResource(config);
expectServiceName(result, "dog-store");
Expand All @@ -67,7 +68,8 @@ void propertiesFileInCurrentDir() throws Exception {

@Test
void classpathApplicationYaml() {
when(system.openClasspathResource(APPLICATION_YML)).thenCallRealMethod();
when(system.openClasspathResource(APPLICATION_YML))
.thenReturn(openClasspathResource(APPLICATION_YML));
SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector(system);
Resource result = guesser.createResource(config);
expectServiceName(result, "cat-store");
Expand Down Expand Up @@ -156,4 +158,8 @@ private static String readString(Path path) throws Exception {
byte[] allBytes = Files.readAllBytes(path);
return new String(allBytes, UTF_8);
}

private InputStream openClasspathResource(String resource) {
return getClass().getClassLoader().getResourceAsStream(resource);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
plugins {
id("otel.java-conventions")
}

dependencies {
testCompileOnly("com.google.auto.service:auto-service-annotations")

testImplementation("org.mockito:mockito-junit-jupiter")
testImplementation(project(":instrumentation:spring:spring-boot-resources:library"))
testImplementation("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure-spi")
testImplementation(project(path = ":smoke-tests:images:spring-boot", configuration = "springBootJar"))
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.instrumentation.spring.resources;

import static io.opentelemetry.semconv.resource.attributes.ResourceAttributes.SERVICE_NAME;
import static org.assertj.core.api.Assertions.assertThat;

import io.opentelemetry.sdk.autoconfigure.spi.ConfigProperties;
import io.opentelemetry.sdk.resources.Resource;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class TestBootInfClassesResource {
@Mock ConfigProperties config;

@Test
void testServiceName() {
// verify that the test app, that is added as a dependency to this project, has the expected
// layout
assertThat(getClass().getResource("/application.properties")).isNull();
assertThat(getClass().getResource("/BOOT-INF/classes/application.properties")).isNotNull();

SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector();
Resource result = guesser.createResource(config);
Comment on lines +29 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector();
Resource result = guesser.createResource(config);
SpringBootServiceNameDetector detector = new SpringBootServiceNameDetector();
Resource result = detector.createResource(config);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copy pasted from

SpringBootServiceNameDetector guesser = new SpringBootServiceNameDetector(system);
Resource result = guesser.createResource(config);

WDYT about renaming this in both tests after this is merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sure. I think guesser was in the original name probably. No biggie.

assertThat(result.getAttribute(SERVICE_NAME)).isEqualTo("otel-spring-test-app");
}
}
1 change: 1 addition & 0 deletions settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ hideFromDependabot(":instrumentation:spark-2.3:javaagent")
hideFromDependabot(":instrumentation:spring:spring-batch-3.0:javaagent")
hideFromDependabot(":instrumentation:spring:spring-boot-actuator-autoconfigure-2.0:javaagent")
hideFromDependabot(":instrumentation:spring:spring-boot-resources:library")
hideFromDependabot(":instrumentation:spring:spring-boot-resources:testing")
hideFromDependabot(":instrumentation:spring:spring-core-2.0:javaagent")
hideFromDependabot(":instrumentation:spring:spring-data:spring-data-1.8:javaagent")
hideFromDependabot(":instrumentation:spring:spring-data:spring-data-3.0:testing")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ abstract class SmokeTest extends Specification {
return Collections.emptyMap()
}

/**
* Subclasses can override this method to disable setting default service name
*/
protected boolean getSetServiceName() {
return true
}

/**
* Subclasses can override this method to provide additional files to copy to target container
*/
Expand All @@ -77,7 +84,7 @@ abstract class SmokeTest extends Specification {

def startTarget(String jdk, String serverVersion, boolean windows) {
def targetImage = getTargetImage(jdk, serverVersion, windows)
return containerManager.startTarget(targetImage, agentPath, jvmArgsEnvVarName, extraEnv, extraResources, extraPorts, getWaitStrategy(), getCommand())
return containerManager.startTarget(targetImage, agentPath, jvmArgsEnvVarName, extraEnv, setServiceName, extraResources, extraPorts, getWaitStrategy(), getCommand())
}

protected abstract String getTargetImage(String jdk)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ import static java.util.stream.Collectors.toSet
class SpringBootSmokeTest extends SmokeTest {

protected String getTargetImage(String jdk) {
"ghcr.io/open-telemetry/opentelemetry-java-instrumentation/smoke-test-spring-boot:jdk$jdk-20211213.1570880324"
"ghcr.io/open-telemetry/opentelemetry-java-instrumentation/smoke-test-spring-boot:jdk$jdk-20230321.4484174638"
}

@Override
protected boolean getSetServiceName() {
return false
}

@Override
Expand Down Expand Up @@ -82,10 +87,17 @@ class SpringBootSmokeTest extends SmokeTest {
metrics.hasMetricsNamed("process.runtime.jvm.memory.committed")
metrics.hasMetricsNamed("process.runtime.jvm.memory.limit")

then: "service name is autodetected"
def serviceName = findResourceAttribute(traces, "service.name")
.map { it.stringValue }
.findAny()
serviceName.isPresent()
serviceName.get() == "otel-spring-test-app"

cleanup:
stopTarget()

where:
jdk << [8, 11, 17]
jdk << [8, 11, 17, 19]
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ public abstract class AbstractTestContainerManager implements TestContainerManag

private boolean started = false;

protected Map<String, String> getAgentEnvironment(String jvmArgsEnvVarName) {
protected Map<String, String> getAgentEnvironment(
String jvmArgsEnvVarName, boolean setServiceName) {
Map<String, String> environment = new HashMap<>();
// while modern JVMs understand linux container memory limits, they do not understand windows
// container memory limits yet, so we need to explicitly set max heap in order to prevent the
Expand All @@ -27,7 +28,9 @@ protected Map<String, String> getAgentEnvironment(String jvmArgsEnvVarName) {
environment.put("OTEL_BSP_SCHEDULE_DELAY", "10ms");
environment.put("OTEL_METRIC_EXPORT_INTERVAL", "1000");
environment.put("OTEL_EXPORTER_OTLP_ENDPOINT", "http://" + BACKEND_ALIAS + ":8080");
environment.put("OTEL_RESOURCE_ATTRIBUTES", "service.name=smoke-test");
if (setServiceName) {
environment.put("OTEL_RESOURCE_ATTRIBUTES", "service.name=smoke-test");
}
environment.put("OTEL_JAVAAGENT_DEBUG", "true");
return environment;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public Consumer<OutputFrame> startTarget(
String agentPath,
String jvmArgsEnvVarName,
Map<String, String> extraEnv,
boolean setServiceName,
List<ResourceMapping> extraResources,
List<Integer> extraPorts,
TargetWaitStrategy waitStrategy,
Expand All @@ -94,7 +95,7 @@ public Consumer<OutputFrame> startTarget(
.withLogConsumer(new Slf4jLogConsumer(appLogger))
.withCopyFileToContainer(
MountableFile.forHostPath(agentPath), "/" + TARGET_AGENT_FILENAME)
.withEnv(getAgentEnvironment(jvmArgsEnvVarName))
.withEnv(getAgentEnvironment(jvmArgsEnvVarName, setServiceName))
.withEnv(extraEnv);

for (ResourceMapping resource : extraResources) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Consumer<OutputFrame> startTarget(
String agentPath,
String jvmArgsEnvVarName,
Map<String, String> extraEnv,
boolean setServiceName,
List<ResourceMapping> extraResources,
List<Integer> extraPorts,
TargetWaitStrategy waitStrategy,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public Consumer<OutputFrame> startTarget(
String agentPath,
String jvmArgsEnvVarName,
Map<String, String> extraEnv,
boolean setServiceName,
List<ResourceMapping> extraResources,
List<Integer> extraPorts,
TargetWaitStrategy waitStrategy,
Expand All @@ -148,7 +149,7 @@ public Consumer<OutputFrame> startTarget(
}

List<String> environment = new ArrayList<>();
getAgentEnvironment(jvmArgsEnvVarName)
getAgentEnvironment(jvmArgsEnvVarName, setServiceName)
.forEach((key, value) -> environment.add(key + "=" + value));
extraEnv.forEach((key, value) -> environment.add(key + "=" + value));

Expand Down