From 4f7888c80e6c5cab85efcecb4109903ea71dbe3c Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 7 Jan 2025 14:14:48 +1100 Subject: [PATCH 01/17] Extract method bodies for easier profiling --- .../collecting/InvocationRegistry.java | 24 ++++++++++--------- .../collecting/InvocationTracker.java | 6 ++++- 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java index c68bb694..b921fc86 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java @@ -28,20 +28,22 @@ public InvocationRegistry() { Thread worker = ScavengerThreadFactory.builder() .name("registry") .build() - .newThread(new Thread(() -> { - while (!Thread.currentThread().isInterrupted()) { - try { - invocations[currentInvocationIndex].add(queue.take()); - } catch (InterruptedException e) { - log.fine("[scavenger] Interrupted"); - Thread.currentThread().interrupt(); - return; - } - } - })); + .newThread(new Thread(this::workerTask)); worker.start(); } + void workerTask() { + while (!Thread.currentThread().isInterrupted()) { + try { + invocations[currentInvocationIndex].add(queue.take()); + } catch (InterruptedException e) { + log.fine("[scavenger] Interrupted"); + Thread.currentThread().interrupt(); + return; + } + } + } + public void register(String hash) { if (!invocations[currentInvocationIndex].contains(hash)) { queue.add(hash); diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java index c29c63f5..40f9e30a 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java @@ -67,7 +67,11 @@ public void onTransformation( @SuppressWarnings("unused") @Advice.OnMethodEnter - public static void onInvocation(@Advice.Origin String signature) { + static void onInvocation(@Advice.Origin String signature) { + hashAndRegister(signature); + } + + public static void hashAndRegister(String signature) { String hash = getMethodRegistry().getHash(signature, isLegacyCompatibilityMode()); //noinspection StringEquality From acd13bc9646550afd9540ee49cdd45ddaa8dc185 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 4 Oct 2024 22:32:47 +1000 Subject: [PATCH 02/17] Upgrade Shadow plugin --- scavenger-agent-java/build.gradle.kts | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/scavenger-agent-java/build.gradle.kts b/scavenger-agent-java/build.gradle.kts index dc3c2b3f..94ccdd67 100644 --- a/scavenger-agent-java/build.gradle.kts +++ b/scavenger-agent-java/build.gradle.kts @@ -1,11 +1,10 @@ -import com.github.jengelman.gradle.plugins.shadow.tasks.ConfigureShadowRelocation import com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar plugins { java `maven-publish` signing - id("com.github.johnrengelman.shadow") version "8.0.0" + id("com.gradleup.shadow") version "8.3.3" id("io.freefair.lombok") version "8.6" id("org.unbroken-dome.test-sets") version "4.1.0" } @@ -27,7 +26,9 @@ tasks.withType { attributes["Implementation-Version"] = project.version } - dependsOn("relocateShadowJar") + isEnableRelocation = true + relocationPrefix = "sc" + mergeServiceFiles() minimize { @@ -38,11 +39,6 @@ tasks.withType { exclude("**/*.kotlin_*") } -tasks.register("relocateShadowJar") { - target = tasks.shadowJar.get() - prefix = "sc" -} - tasks.assemble { dependsOn(tasks.shadowJar) } From c4706088456f8779a57cf35a35c0efdfb44fea6d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 4 Oct 2024 23:34:51 +1000 Subject: [PATCH 03/17] Extract WireMock setup to abstract class --- .../javaagent/AbstractWireMockTest.java | 37 +++++++++++++++++++ .../javaagent/InvocationTest.java | 22 +---------- .../integrationTest/javaagent/ScanTest.java | 22 +---------- 3 files changed, 39 insertions(+), 42 deletions(-) create mode 100644 scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/AbstractWireMockTest.java diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/AbstractWireMockTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/AbstractWireMockTest.java new file mode 100644 index 00000000..ea47d747 --- /dev/null +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/AbstractWireMockTest.java @@ -0,0 +1,37 @@ +package integrationTest.javaagent; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.client.WireMock; +import com.github.tomakehurst.wiremock.core.WireMockConfiguration; +import io.grpc.ManagedChannel; +import io.grpc.ManagedChannelBuilder; +import org.grpcmock.GrpcMock; +import org.grpcmock.junit5.GrpcMockExtension; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.extension.ExtendWith; + +import java.util.Optional; + +@ExtendWith(GrpcMockExtension.class) +public class AbstractWireMockTest { + + protected static WireMockServer wireMockServer; + protected static ManagedChannel channel; + + @BeforeAll + static void startWireMockServer() { + wireMockServer = new WireMockServer(new WireMockConfiguration().dynamicPort()); + wireMockServer.start(); + WireMock.configureFor(wireMockServer.port()); + channel = ManagedChannelBuilder.forAddress("localhost", GrpcMock.getGlobalPort()) + .usePlaintext() + .build(); + } + + @AfterAll + static void shutDownWireMockServer() { + Optional.ofNullable(wireMockServer).ifPresent(WireMockServer::shutdown); + Optional.ofNullable(channel).ifPresent(ManagedChannel::shutdownNow); + } +} diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java index 0f16524e..f19030e2 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java @@ -47,28 +47,8 @@ import com.navercorp.scavenger.model.PublicationResponse; @ExtendWith(AgentIntegrationTestContextProvider.class) -@ExtendWith(GrpcMockExtension.class) @DisplayName("invocation track test") -public class InvocationTest { - private static WireMockServer wireMockServer; - private static ManagedChannel channel; - - @BeforeAll - static void setUp() { - wireMockServer = new WireMockServer(new WireMockConfiguration().dynamicPort()); - wireMockServer.start(); - WireMock.configureFor(wireMockServer.port()); - - channel = ManagedChannelBuilder.forAddress("localhost", GrpcMock.getGlobalPort()) - .usePlaintext() - .build(); - } - - @AfterAll - static void tearDown() { - Optional.ofNullable(wireMockServer).ifPresent(WireMockServer::shutdown); - Optional.ofNullable(channel).ifPresent(ManagedChannel::shutdownNow); - } +public class InvocationTest extends AbstractWireMockTest { @TestTemplate @DisplayName("it tracks correctly") diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java index dcbd5eec..8ba4894a 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java @@ -50,28 +50,8 @@ import sample.app.regex.ExcludeRegexClass2; @ExtendWith(AgentIntegrationTestContextProvider.class) -@ExtendWith(GrpcMockExtension.class) @DisplayName("codebase scan test") -public class ScanTest { - private static WireMockServer wireMockServer; - private static ManagedChannel channel; - - @BeforeAll - static void setUp() { - wireMockServer = new WireMockServer(new WireMockConfiguration().dynamicPort()); - wireMockServer.start(); - WireMock.configureFor(wireMockServer.port()); - - channel = ManagedChannelBuilder.forAddress("localhost", GrpcMock.getGlobalPort()) - .usePlaintext() - .build(); - } - - @AfterAll - static void tearDown() { - Optional.ofNullable(wireMockServer).ifPresent(WireMockServer::shutdown); - Optional.ofNullable(channel).ifPresent(ManagedChannel::shutdownNow); - } +public class ScanTest extends AbstractWireMockTest { @TestTemplate @DisplayName("it scans correctly") From ca5069635ac59cbaeb5049a267fa9e26a867adba Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 5 Oct 2024 16:24:21 +1000 Subject: [PATCH 04/17] Implement JMH benchmarks --- scavenger-agent-java/build.gradle.kts | 4 ++ .../javaagent/BenchmarkTest.java | 30 +++++++++ .../integrationTest/javaagent/ConfigTest.java | 6 +- .../javaagent/InvocationTest.java | 6 +- .../integrationTest/javaagent/ScanTest.java | 6 +- .../support/AgentBenchmarkExtension.java | 14 ++++ .../AgentIntegrationTestContextProvider.java | 15 +++-- .../integrationTest/support/AgentRunner.java | 64 +++++++++++-------- .../java/sample/app/NotServiceClass.java | 4 ++ .../app/excluded/jmh/BenchmarkRunner.java | 27 ++++++++ .../resources/scavenger-jmh.conf | 7 ++ 11 files changed, 142 insertions(+), 41 deletions(-) create mode 100644 scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java create mode 100644 scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentBenchmarkExtension.java create mode 100644 scavenger-agent-java/src/integrationTest/java/sample/app/excluded/jmh/BenchmarkRunner.java create mode 100644 scavenger-agent-java/src/integrationTest/resources/scavenger-jmh.conf diff --git a/scavenger-agent-java/build.gradle.kts b/scavenger-agent-java/build.gradle.kts index 94ccdd67..5c3fb730 100644 --- a/scavenger-agent-java/build.gradle.kts +++ b/scavenger-agent-java/build.gradle.kts @@ -81,6 +81,10 @@ dependencies { "integrationTestImplementation"("org.springframework.boot:spring-boot-starter-aop:2.5.12") "integrationTestImplementation"("com.github.tomakehurst:wiremock:2.27.2") "integrationTestImplementation"("org.grpcmock:grpcmock-junit5:0.13.0") + + // JMH benchmarks + "integrationTestImplementation"("org.openjdk.jmh:jmh-core:1.37") + "integrationTestAnnotationProcessor"("org.openjdk.jmh:jmh-generator-annprocess:1.37") } fun javaPaths(vararg versions: Int) = versions.joinToString(",", diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java new file mode 100644 index 00000000..e2287692 --- /dev/null +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java @@ -0,0 +1,30 @@ +package integrationTest.javaagent; + +import integrationTest.support.AgentBenchmarkExtension; +import integrationTest.support.AgentRunner; +import lombok.extern.java.Log; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.TestTemplate; +import org.junit.jupiter.api.extension.ExtendWith; + +@Log +@Disabled("Comment/remove this annotation to run benchmarks") +@ExtendWith(AgentBenchmarkExtension.class) +public class BenchmarkTest { + + @TestTemplate + @DisplayName("JMH benchmark") + void bench(AgentRunner agentRunner) throws Exception { + agentRunner.setShouldLogOutput(true); + agentRunner.call(); + } + + @TestTemplate + @DisplayName("JMH benchmark no advice") + void benchNoAdvice(AgentRunner agentRunner) throws Exception { + agentRunner.setConfigProperty("packages", "none"); + agentRunner.setShouldLogOutput(true); + agentRunner.call(); + } +} diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ConfigTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ConfigTest.java index 48fe47d1..0503c398 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ConfigTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ConfigTest.java @@ -20,7 +20,7 @@ public class ConfigTest { @DisplayName("if no configuration is found") void noConfig(AgentRunner agentRunner) throws Exception { // given - agentRunner.setConfigFilePath(""); + agentRunner.setConfigFilePath(null); // when String actual = agentRunner.call(); @@ -50,9 +50,7 @@ void nonExistentConfig(AgentRunner agentRunner) throws Exception { @DisplayName("if required field is not set") void missingRequiredTest(AgentRunner agentRunner) throws Exception { // given - Properties properties = new Properties(); - properties.setProperty("packages", ""); - agentRunner.setConfig(properties); + agentRunner.setConfigProperty("packages", ""); // when String actual = agentRunner.call(); diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java index f19030e2..d12a010d 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/InvocationTest.java @@ -68,10 +68,8 @@ void track(AgentRunner agentRunner) throws Exception { @DisplayName("it sends publication correctly") void send(AgentRunner agentRunner) throws Exception { // given - Properties properties = new Properties(); - properties.setProperty("serverUrl", "http://localhost:" + wireMockServer.port()); - properties.setProperty("schedulerInitialDelayMillis", "0"); - agentRunner.setConfig(properties); + agentRunner.setConfigProperty("serverUrl", "http://localhost:" + wireMockServer.port()); + agentRunner.setConfigProperty("schedulerInitialDelayMillis", "0"); givenThat( get(V5_INIT_CONFIG + "?licenseKey=") diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java index 8ba4894a..90f4a9ea 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java @@ -73,10 +73,8 @@ void scan(AgentRunner agentRunner) throws Exception { @DisplayName("it sends publication correctly") void send(AgentRunner agentRunner) throws Exception { // given - Properties properties = new Properties(); - properties.setProperty("schedulerInitialDelayMillis", "0"); - properties.setProperty("serverUrl", "http://localhost:" + wireMockServer.port()); - agentRunner.setConfig(properties); + agentRunner.setConfigProperty("serverUrl", "http://localhost:" + wireMockServer.port()); + agentRunner.setConfigProperty("schedulerInitialDelayMillis", "0"); givenThat( get(V5_INIT_CONFIG + "?licenseKey=") diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentBenchmarkExtension.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentBenchmarkExtension.java new file mode 100644 index 00000000..c32030aa --- /dev/null +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentBenchmarkExtension.java @@ -0,0 +1,14 @@ +package integrationTest.support; + +public class AgentBenchmarkExtension extends AgentIntegrationTestContextProvider { + + @Override + protected Class getTestAppMainClass() { + return org.openjdk.jmh.Main.class; + } + + @Override + protected String getScavengerConfigPath() { + return "scavenger-jmh.conf"; + } +} diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentIntegrationTestContextProvider.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentIntegrationTestContextProvider.java index fb8a706c..d8c948c2 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentIntegrationTestContextProvider.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentIntegrationTestContextProvider.java @@ -33,6 +33,14 @@ public Stream provideTestTemplateInvocationContex return Arrays.stream(javaPaths.split(",")).map(this::invocationContext); } + protected Class getTestAppMainClass() { + return sample.app.SampleApp.class; + } + + protected String getScavengerConfigPath() { + return "scavenger.conf"; + } + private TestTemplateInvocationContext invocationContext(String javaPathString) { String[] split = javaPathString.split(":"); String javaVersion = split[0]; @@ -51,7 +59,7 @@ public List getAdditionalExtensions() { }; } - private static class AgentRunnerParameterResolver implements ParameterResolver { + private class AgentRunnerParameterResolver implements ParameterResolver { private final String javaPath; private AgentRunnerParameterResolver(String javaPath) { @@ -60,13 +68,12 @@ private AgentRunnerParameterResolver(String javaPath) { @Override public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException { - return parameterContext.getParameter().getType() - .equals(AgentRunner.class); + return parameterContext.getParameter().getType().equals(AgentRunner.class); } @Override public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException { - return new AgentRunner(javaPath, classpath, scavengerAgentPath); + return new AgentRunner(javaPath, classpath, getTestAppMainClass(), scavengerAgentPath, getScavengerConfigPath()); } } } diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentRunner.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentRunner.java index d4a374a3..9fed77d2 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentRunner.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/support/AgentRunner.java @@ -1,37 +1,40 @@ package integrationTest.support; +import static java.util.stream.Collectors.joining; + +import lombok.Setter; + import java.io.BufferedReader; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; -import java.util.Properties; +import java.util.Map; import java.util.concurrent.Callable; -import java.util.stream.Collectors; public class AgentRunner implements Callable { private final String javaPath; private final String classpath; + private final String mainClass; private final String agentPath; - + private final Map configProps = new HashMap<>(); + @Setter + private boolean shouldLogOutput; + @Setter private String configFilePath; - private String cmdLineArgs; - public AgentRunner(String javaPath, String classpath, String agentPath) { + public AgentRunner(String javaPath, String classpath, Class mainClass, String agentPath, String configFilePath) { this.javaPath = javaPath; this.classpath = classpath; + this.mainClass = mainClass.getName(); this.agentPath = agentPath; - } - - public void setConfigFilePath(String configFilePath) { this.configFilePath = configFilePath; } - public void setConfig(Properties properties) { - cmdLineArgs = properties.entrySet().stream() - .map(entry -> entry.getKey() + "=" + entry.getValue()) - .collect(Collectors.joining(";")); + public void setConfigProperty(String key, String value) { + configProps.put(key, value); } @Override @@ -47,33 +50,44 @@ public String call() throws IOException, InterruptedException, RuntimeException } private List buildCommand() { - String cp = classpath.endsWith(":") ? classpath.substring(0, classpath.length() - 2) : classpath; - List command = new ArrayList<>(); command.add(javaPath); - if (cmdLineArgs != null) { - command.add("-javaagent:" + agentPath + "=" + cmdLineArgs); - } else { - command.add("-javaagent:" + agentPath); + String agentCmd = "-javaagent:" + agentPath; + if (!configProps.isEmpty()) { + agentCmd += "=" + cmdLineArgsToString(); } + command.add(agentCmd); command.add("-cp"); + String cp = classpath.endsWith(":") ? classpath.substring(0, classpath.length() - 2) : classpath; command.add(cp); command.add("-Djava.util.logging.config.file=src/integrationTest/resources/logging.properties"); command.add("-Duser.language=en"); command.add("-Duser.country=US"); - if (configFilePath == null) { - command.add("-Dscavenger.configuration=src/integrationTest/resources/scavenger.conf"); - } else if (!configFilePath.isEmpty()) { - command.add("-Dscavenger.configuration=" + configFilePath); + if (configFilePath != null) { + command.add("-Dscavenger.configuration=src/integrationTest/resources/" + configFilePath); } - command.add("sample.app.SampleApp"); + command.add(mainClass); System.out.println("Launching SampleApp with the command: " + command); return command; } - private static String collectProcessOutput(InputStream inputStream) throws IOException { + private String cmdLineArgsToString() { + return configProps.entrySet().stream() + .map(entry -> entry.getKey() + "=" + entry.getValue()) + .collect(joining(";")); + } + + private String collectProcessOutput(InputStream inputStream) throws IOException { + StringBuilder output = new StringBuilder(); try (BufferedReader reader = new BufferedReader(new InputStreamReader(inputStream))) { - return reader.lines().collect(Collectors.joining("\n")); + String line; + while ((line = reader.readLine()) != null) { + if (shouldLogOutput) { + System.out.println(line); + } + output.append(line).append("\n"); + } } + return output.toString(); } } diff --git a/scavenger-agent-java/src/integrationTest/java/sample/app/NotServiceClass.java b/scavenger-agent-java/src/integrationTest/java/sample/app/NotServiceClass.java index cf19bdd0..32a61f87 100644 --- a/scavenger-agent-java/src/integrationTest/java/sample/app/NotServiceClass.java +++ b/scavenger-agent-java/src/integrationTest/java/sample/app/NotServiceClass.java @@ -8,4 +8,8 @@ public class NotServiceClass { public static void doSomething(int p1) { log.info("Doing something " + p1); } + + public static void doNothing() { + // Intentionally left blank + } } diff --git a/scavenger-agent-java/src/integrationTest/java/sample/app/excluded/jmh/BenchmarkRunner.java b/scavenger-agent-java/src/integrationTest/java/sample/app/excluded/jmh/BenchmarkRunner.java new file mode 100644 index 00000000..23b4679a --- /dev/null +++ b/scavenger-agent-java/src/integrationTest/java/sample/app/excluded/jmh/BenchmarkRunner.java @@ -0,0 +1,27 @@ +package sample.app.excluded.jmh; + +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; +import sample.app.NotServiceClass; + +import java.util.concurrent.TimeUnit; + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Fork(2) +@Threads(4) +@Warmup(iterations = 2) +@Measurement(iterations = 3) +public class BenchmarkRunner { + + @Benchmark + public void bench() { + NotServiceClass.doNothing(); + } +} diff --git a/scavenger-agent-java/src/integrationTest/resources/scavenger-jmh.conf b/scavenger-agent-java/src/integrationTest/resources/scavenger-jmh.conf new file mode 100644 index 00000000..e77bd002 --- /dev/null +++ b/scavenger-agent-java/src/integrationTest/resources/scavenger-jmh.conf @@ -0,0 +1,7 @@ +appName = SampleApp +packages = sample +methodVisibility = protected +excludePackages = sample.app.excluded +codeBase = build/classes/java/integrationTest +debugMode = false +schedulerInitialDelayMillis = 1000000 From ceb6de72bb0d50a01524d65b15c235bd37107a9d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 5 Oct 2024 16:47:05 +1000 Subject: [PATCH 05/17] Log advice installed --- .../scavenger/javaagent/collecting/InvocationTracker.java | 1 + 1 file changed, 1 insertion(+) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java index 40f9e30a..7f490e2c 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java @@ -63,6 +63,7 @@ public void onTransformation( }); } transform.installOn(inst); + log.info("[scavenger] Advice is installed on all matching methods"); } @SuppressWarnings("unused") From 09025dcac3d58cabf5aff6ad48f10d65ba84e22a Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 5 Oct 2024 21:25:33 +1000 Subject: [PATCH 06/17] #extractSignature optimisation --- .../javaagent/collecting/MethodRegistry.java | 13 +++++-------- .../javaagent/collecting/MethodRegistryTest.java | 7 +++++++ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java index a6990cf7..3cec4e72 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java @@ -36,23 +36,20 @@ public String getHash(String byteBuddySignature, boolean legacyCompatibilityMode return hash; } - private String extractSignature(String byteBuddySignature) { + static String extractSignature(String byteBuddySignature) { int begin = 0; int end = byteBuddySignature.length(); boolean isParenthesisFound = false; - - for (int pos = 0; pos < end; pos++) { - if (byteBuddySignature.charAt(pos) == ' ') { - if (!isParenthesisFound) { + for (int pos = end - 1; pos >= 0; pos--) { + if (isParenthesisFound) { + if (byteBuddySignature.charAt(pos) == ' ') { begin = pos + 1; - } else { - end = pos; + break; } } else if (byteBuddySignature.charAt(pos) == '(') { isParenthesisFound = true; } } - return byteBuddySignature.substring(begin, end); } } diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java index 304302e8..37dbcc71 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java @@ -53,4 +53,11 @@ void returnNull() { } } + @Test + void extractSignature() { + assertThat( + MethodRegistry.extractSignature("public static void sample.app.NotServiceClass.doNothing()")) + .isEqualTo("sample.app.NotServiceClass.doNothing()"); + } + } From b5c1ad80dbcb49e7bb69f25a29601d87d3731c17 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 2 Oct 2024 19:03:08 +1000 Subject: [PATCH 07/17] Refactor signature hashing --- .../collecting/InvocationTracker.java | 41 ++++++----------- .../javaagent/collecting/MethodRegistry.java | 44 +++++++++++-------- .../collecting/MethodRegistryTest.java | 6 +-- 3 files changed, 42 insertions(+), 49 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java index 7f490e2c..17b345af 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java @@ -1,7 +1,8 @@ package com.navercorp.scavenger.javaagent.collecting; +import static com.navercorp.scavenger.javaagent.collecting.MethodRegistry.isSyntheticSignatureHash; + import java.lang.instrument.Instrumentation; -import java.util.logging.Logger; import net.bytebuddy.ByteBuddy; import net.bytebuddy.agent.builder.AgentBuilder; @@ -22,25 +23,12 @@ public class InvocationTracker { @Getter @Setter private static InvocationRegistry invocationRegistry = new InvocationRegistry(); - - @Getter - private static final MethodRegistry methodRegistry = new MethodRegistry(); - - @Setter - @Getter - private static boolean legacyCompatibilityMode = false; - - @Setter - @Getter - private static boolean debugMode = false; - - public static Logger getLog() { - return log; - } + private static MethodRegistry methodRegistry; + private static boolean isDebugMode; public static void installAdvice(Instrumentation inst, Config config) { - setLegacyCompatibilityMode(config.isLegacyCompatibilityMode()); - setDebugMode(config.isDebugMode()); + methodRegistry = new MethodRegistry(config.isLegacyCompatibilityMode()); + isDebugMode = config.isDebugMode(); ElementMatcherBuilder matcherBuilder = new ElementMatcherBuilder(config); Advice advice = Advice.to(InvocationTracker.class); @@ -49,7 +37,7 @@ public static void installAdvice(Instrumentation inst, Config config) { .transform((builder, typeDescription, classLoader, module, protectionDomain) -> builder.visit(advice.on(matcherBuilder.buildMethodMatcher(typeDescription))) ); - if (isDebugMode()) { + if (isDebugMode) { transform = transform.with(new AgentBuilder.Listener.Adapter() { @Override public void onTransformation( @@ -73,14 +61,13 @@ static void onInvocation(@Advice.Origin String signature) { } public static void hashAndRegister(String signature) { - String hash = getMethodRegistry().getHash(signature, isLegacyCompatibilityMode()); - - //noinspection StringEquality - if (hash != MethodRegistry.SYNTHETIC_SIGNATURE_HASH) { - if (isDebugMode()) { - getLog().info("[scavenger] method " + signature + " is invoked - " + hash); - } - getInvocationRegistry().register(hash); + String hash = methodRegistry.getHash(signature); + if (isSyntheticSignatureHash(hash)) { + return; + } + if (isDebugMode) { + log.info("[scavenger] method " + signature + " is invoked - " + hash); } + invocationRegistry.register(hash); } } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java index 3cec4e72..b3228ba0 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java @@ -5,35 +5,41 @@ import java.util.regex.Pattern; import lombok.Getter; +import lombok.extern.java.Log; import com.navercorp.scavenger.util.HashGenerator; +@Log public class MethodRegistry { - public static final String SYNTHETIC_SIGNATURE_HASH = ""; + private static final String SYNTHETIC_SIGNATURE_HASH = ""; + private static final Pattern SYNTHETIC_SIGNATURE_PATTERN = Pattern.compile(".*\\$\\$(Enhancer|FastClass)BySpringCGLIB\\$\\$.*"); @Getter private final Map byteBuddySignatureToHash = new ConcurrentHashMap<>(); + private final boolean isLegacyCompatibilityMode; - private static final Pattern SYNTHETIC_SIGNATURE_PATTERN = Pattern.compile(".*\\$\\$(Enhancer|FastClass)BySpringCGLIB\\$\\$.*"); + public MethodRegistry(boolean isLegacyCompatibilityMode) { + this.isLegacyCompatibilityMode = isLegacyCompatibilityMode; + } - public String getHash(String byteBuddySignature, boolean legacyCompatibilityMode) { - String hash = this.byteBuddySignatureToHash.get(byteBuddySignature); - - if (hash == null) { - if (SYNTHETIC_SIGNATURE_PATTERN.matcher(byteBuddySignature).matches()) { - hash = SYNTHETIC_SIGNATURE_HASH; - } else { - String signature = extractSignature(byteBuddySignature); - if (legacyCompatibilityMode) { - signature = signature.replace('$', '.'); - signature = signature.replace(",", ", "); - } - hash = HashGenerator.Md5.from(signature); - } - this.byteBuddySignatureToHash.put(byteBuddySignature, hash); - } + public String getHash(String byteBuddySignature) { + return byteBuddySignatureToHash.computeIfAbsent(byteBuddySignature, this::generateHash); + } + + @SuppressWarnings("StringEquality") + public static boolean isSyntheticSignatureHash(String hash) { + return SYNTHETIC_SIGNATURE_HASH == hash; + } - return hash; + private String generateHash(String byteBuddySignature) { + if (SYNTHETIC_SIGNATURE_PATTERN.matcher(byteBuddySignature).matches()) { + return SYNTHETIC_SIGNATURE_HASH; + } + String signature = extractSignature(byteBuddySignature); + if (isLegacyCompatibilityMode) { + signature = signature.replace('$', '.').replace(",", ", "); + } + return HashGenerator.Md5.from(signature); } static String extractSignature(String byteBuddySignature) { diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java index 37dbcc71..814546cd 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java @@ -14,7 +14,7 @@ public class MethodRegistryTest { @BeforeEach public void setUp() { - sut = new MethodRegistry(); + sut = new MethodRegistry(false); } @Nested @@ -35,7 +35,7 @@ public void cacheMethod() { @Test @DisplayName("it returns cached value") void cached() { - assertThat(sut.getHash(signature, false)).isEqualTo(expected); + assertThat(sut.getHash(signature)).isEqualTo(expected); } } @@ -47,7 +47,7 @@ class CglibTest { @Test @DisplayName("it returns empty string") void returnNull() { - assertThat(sut.getHash(cglibSignature, false)) + assertThat(sut.getHash(cglibSignature)) .isEqualTo(""); } } From 71773d57b00cd848705584321057f8b6fdf1d272 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Fri, 4 Oct 2024 23:43:24 +1000 Subject: [PATCH 08/17] Filter synthetic signatures at scan time --- .../integrationTest/javaagent/ScanTest.java | 71 +++++++++++-------- .../util/AgentLogAssertionUtil.java | 22 ++++++ .../collecting/ElementMatcherBuilder.java | 3 + .../collecting/InvocationTracker.java | 5 -- .../javaagent/collecting/MethodRegistry.java | 11 --- .../collecting/ElementMatcherBuilderTest.java | 33 +++++++-- .../collecting/MethodRegistryTest.java | 13 ---- 7 files changed, 94 insertions(+), 64 deletions(-) diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java index 90f4a9ea..4fda1193 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/ScanTest.java @@ -5,6 +5,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.okJson; import static com.navercorp.scavenger.model.Endpoints.Agent.V5_INIT_CONFIG; import static integrationTest.util.AgentLogAssertionUtil.assertSampleAppOutput; +import static integrationTest.util.AgentLogAssertionUtil.extractFromMatchingLogLines; import static org.assertj.core.api.Assertions.assertThat; import static org.grpcmock.GrpcMock.calledMethod; import static org.grpcmock.GrpcMock.getGlobalPort; @@ -13,42 +14,30 @@ import static org.grpcmock.GrpcMock.verifyThat; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.lang.reflect.Method; -import java.util.Optional; -import java.util.Properties; +import java.util.List; import java.util.regex.Matcher; import java.util.regex.Pattern; -import org.grpcmock.GrpcMock; -import org.grpcmock.junit5.GrpcMockExtension; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; +import com.navercorp.scavenger.javaagent.collecting.CodeBaseScanner; +import com.navercorp.scavenger.javaagent.collecting.InvocationTracker; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; -import com.github.tomakehurst.wiremock.WireMockServer; -import com.github.tomakehurst.wiremock.client.WireMock; -import com.github.tomakehurst.wiremock.core.WireMockConfiguration; import com.google.protobuf.util.JsonFormat; import integrationTest.support.AgentIntegrationTestContextProvider; import integrationTest.support.AgentRunner; -import integrationTest.util.AgentLogAssertionUtil; -import io.grpc.ManagedChannel; -import io.grpc.ManagedChannelBuilder; import sample.app.NotServiceClass; +import sample.app.SampleApp; +import sample.app.SampleAspect; import sample.app.SampleService1; -import sample.app.excluded.NotTrackedClass; -import sample.app.excluded.NotTrackedClass2; +import sample.app.SampleService2; import com.navercorp.scavenger.model.GetConfigResponse; import com.navercorp.scavenger.model.GrpcAgentServiceGrpc; import com.navercorp.scavenger.model.InitConfigResponse; import com.navercorp.scavenger.model.PublicationResponse; -import sample.app.regex.ExcludeRegexClass1; -import sample.app.regex.ExcludeRegexClass2; - @ExtendWith(AgentIntegrationTestContextProvider.class) @DisplayName("codebase scan test") public class ScanTest extends AbstractWireMockTest { @@ -61,12 +50,39 @@ void scan(AgentRunner agentRunner) throws Exception { // then assertSampleAppOutput(stdout); - assertThat(stdout).matches(scanned(SampleService1.class.getMethod("doSomething", int.class))); - assertThat(stdout).matches(scanned(NotServiceClass.class.getMethod("doSomething", int.class))); - assertThat(stdout).doesNotMatch(scanned(NotTrackedClass.class.getMethod("doSomething"))); - assertThat(stdout).doesNotMatch(scanned(NotTrackedClass2.class.getMethod("doSomething"))); - assertThat(stdout).doesNotMatch(scanned(ExcludeRegexClass1.class.getMethod("doSomething"))); - assertThat(stdout).doesNotMatch(scanned(ExcludeRegexClass2.class.getMethod("doSomething"))); + List scannedMethods = extractFromMatchingLogLines(stdout, CodeBaseScanner.class, "[scavenger] ", " is scanned"); + assertThat(scannedMethods).containsExactlyInAnyOrder( + "sample.app.NotServiceClass()", + "sample.app.NotServiceClass.doNothing()", + "sample.app.NotServiceClass.doSomething(int)", + "sample.app.SampleApp(sample.app.SampleService1)", + "sample.app.SampleApp.add(int,int)", + "sample.app.SampleApp.main(java.lang.String[])", + "sample.app.SampleApp.postConstruct()", + "sample.app.SampleAspect()", + "sample.app.SampleAspect.aroundSampleService(org.aspectj.lang.ProceedingJoinPoint)", + "sample.app.SampleAspect.logAspectLoaded()", + "sample.app.SampleService1(sample.app.SampleService2)", + "sample.app.SampleService1.doSomething(int)", + "sample.app.SampleService2()", + "sample.app.SampleService2.doSomething(int)" + ); + } + + @TestTemplate + @DisplayName("it installs advice correctly") + void advice(AgentRunner agentRunner) throws Exception { + String stdout = agentRunner.call(); + + List installedAdvice = extractFromMatchingLogLines(stdout, InvocationTracker.class, + "[scavenger] Advice on ", " is installed"); + + assertThat(installedAdvice).containsExactlyInAnyOrder( + SampleApp.class.getName(), + SampleService1.class.getName(), + SampleAspect.class.getName(), + SampleService2.class.getName(), + NotServiceClass.class.getName()); } @TestTemplate @@ -111,13 +127,6 @@ void send(AgentRunner agentRunner) throws Exception { .withRequest(pub -> pub.getEntryCount() == getMethodsCount(stdout))); } - private static Pattern scanned(Method method) { - String[] split = method.toString().split(" "); - String signature = split[split.length - 1]; - return AgentLogAssertionUtil.logPattern("com.navercorp.scavenger.javaagent.collecting.CodeBaseScanner", - "[scavenger] " + signature + " is scanned"); - } - private static int getMethodsCount(String stdout) { Matcher matcher = Pattern.compile("\\[scavenger] codebase\\(.*\\) scanned in \\d* ms: (\\d*) methods").matcher(stdout); assertTrue(matcher.find()); diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/util/AgentLogAssertionUtil.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/util/AgentLogAssertionUtil.java index 326a6869..db1f18c5 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/util/AgentLogAssertionUtil.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/util/AgentLogAssertionUtil.java @@ -2,6 +2,9 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; import java.util.regex.Pattern; public class AgentLogAssertionUtil { @@ -22,4 +25,23 @@ public static void assertSampleAppOutput(String stdout) { public static Pattern logPattern(String location, String text) { return Pattern.compile("[\\s\\S]*(INFO).*" + Pattern.quote(location) + ".*" + Pattern.quote(text) + "[\\s\\S]*"); } + + public static List extractFromMatchingLogLines(String multiLineLogOutput, Class loggingClass, String matchingPrefix, String matchingSuffix) { + return extractFromMatchingLogLines(multiLineLogOutput, loggingClass, Pattern.quote(matchingPrefix) + "(.*)" + Pattern.quote(matchingSuffix)); + } + + public static List extractFromMatchingLogLines(String multiLineLogOutput, Class loggingClass, String messageRegexWithCaptureGroup) { + List result = new ArrayList<>(); + String loggerName = loggingClass.getName(); + String logLineRegex = ".*?\\[.*?] " + Pattern.quote(loggerName) + " - " + messageRegexWithCaptureGroup; + Pattern logLinePattern = Pattern.compile(logLineRegex, Pattern.MULTILINE); + Matcher matcher = logLinePattern.matcher(multiLineLogOutput); + while (matcher.find()) { + String capturedValue = matcher.group(1); + if (capturedValue != null && !capturedValue.isEmpty()) { + result.add(capturedValue); + } + } + return result; + } } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilder.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilder.java index 41106b1a..0dc27c22 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilder.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilder.java @@ -35,6 +35,8 @@ @RequiredArgsConstructor public class ElementMatcherBuilder { + private static final String SYNTHETIC_REGEX = ".*\\$\\$(Enhancer|FastClass)BySpringCGLIB\\$\\$.*"; + private final Config config; public ElementMatcher buildClassMatcher() { @@ -72,6 +74,7 @@ public ElementMatcher buildClassMatcher() { return packageNameMatcher .and(not(isSynthetic())) + .and(not(nameMatches(SYNTHETIC_REGEX))) .and(not(isInterface())) .and(not(excludePackageMatcher)) .and(annotationMatcher.or(additionalPackageMatcher).or(additionalByRegexMatcher)) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java index 17b345af..21387e4c 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java @@ -1,7 +1,5 @@ package com.navercorp.scavenger.javaagent.collecting; -import static com.navercorp.scavenger.javaagent.collecting.MethodRegistry.isSyntheticSignatureHash; - import java.lang.instrument.Instrumentation; import net.bytebuddy.ByteBuddy; @@ -62,9 +60,6 @@ static void onInvocation(@Advice.Origin String signature) { public static void hashAndRegister(String signature) { String hash = methodRegistry.getHash(signature); - if (isSyntheticSignatureHash(hash)) { - return; - } if (isDebugMode) { log.info("[scavenger] method " + signature + " is invoked - " + hash); } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java index b3228ba0..64d9aec9 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java @@ -2,7 +2,6 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.regex.Pattern; import lombok.Getter; import lombok.extern.java.Log; @@ -11,8 +10,6 @@ @Log public class MethodRegistry { - private static final String SYNTHETIC_SIGNATURE_HASH = ""; - private static final Pattern SYNTHETIC_SIGNATURE_PATTERN = Pattern.compile(".*\\$\\$(Enhancer|FastClass)BySpringCGLIB\\$\\$.*"); @Getter private final Map byteBuddySignatureToHash = new ConcurrentHashMap<>(); @@ -26,15 +23,7 @@ public String getHash(String byteBuddySignature) { return byteBuddySignatureToHash.computeIfAbsent(byteBuddySignature, this::generateHash); } - @SuppressWarnings("StringEquality") - public static boolean isSyntheticSignatureHash(String hash) { - return SYNTHETIC_SIGNATURE_HASH == hash; - } - private String generateHash(String byteBuddySignature) { - if (SYNTHETIC_SIGNATURE_PATTERN.matcher(byteBuddySignature).matches()) { - return SYNTHETIC_SIGNATURE_HASH; - } String signature = extractSignature(byteBuddySignature); if (isLegacyCompatibilityMode) { signature = signature.replace('$', '.').replace(",", ", "); diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilderTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilderTest.java index 59f5b8c3..64b34bb4 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilderTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/ElementMatcherBuilderTest.java @@ -2,6 +2,7 @@ import static org.assertj.core.api.Assertions.assertThat; +import java.lang.annotation.Annotation; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -16,6 +17,7 @@ import net.bytebuddy.description.annotation.AnnotationDescription; import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription.Generic.OfNonGenericType; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.scaffold.InstrumentedType; import net.bytebuddy.matcher.ElementMatcher; @@ -26,7 +28,7 @@ @DisplayName("ElementMatcherBuilder class") public class ElementMatcherBuilderTest { TypeDescription withClazzNameWithPackage(String clazzName) { - return InstrumentedType.Default.of(clazzName, TypeDescription.Generic.OBJECT, 0); + return InstrumentedType.Default.of(clazzName, OfNonGenericType.ForLoadedType.of(Object.class), 0); } TypeDescription withAnnotations(String clazzName, String annotationName) { @@ -37,22 +39,45 @@ TypeDescription withAnnotations(String clazzName, List annotationNames) List annotations = annotationNames.stream() .map(annotationName -> AnnotationDescription.Builder.ofType( - InstrumentedType.Default.of(annotationName, TypeDescription.Generic.ANNOTATION, Opcodes.ACC_ANNOTATION) + InstrumentedType.Default.of(annotationName, OfNonGenericType.ForLoadedType.of(Annotation.class), Opcodes.ACC_ANNOTATION) ).build() ).collect(Collectors.toList()); - return InstrumentedType.Default.of(clazzName, TypeDescription.Generic.OBJECT, 0) + return InstrumentedType.Default.of(clazzName, OfNonGenericType.ForLoadedType.of(Object.class), 0) .withAnnotations(annotations); } MethodDescription withModifiers(int modifiers) { - return new MethodDescription.Latent(TypeDescription.OBJECT, new MethodDescription.Token(modifiers)); + return new MethodDescription.Latent(TypeDescription.ForLoadedType.of(Object.class), new MethodDescription.Token(modifiers)); } @Nested @DisplayName("buildClassMatcher method") class BuildClassMatcherMethodTest { + @Nested + @DisplayName("when synthetic classes are present") + class SyntheticMethodTest { + ElementMatcher matcher; + + @BeforeEach + public void prepareMatcher() { + Properties props = new Properties(); + props.setProperty("packages", "com"); + Config config = new Config(props); + ElementMatcherBuilder builder = new ElementMatcherBuilder(config); + matcher = builder.buildClassMatcher(); + } + + @Test + @DisplayName("it returns false for synthetic class methods") + void emptyString() { + assertThat(matcher.matches(withClazzNameWithPackage("com.pkg.SampleService2$$FastClassBySpringCGLIB$$7c08ba38"))).isFalse(); + assertThat(matcher.matches(withClazzNameWithPackage("com.pkg.SampleService1$$EnhancerBySpringCGLIB$$1cdd7e7b"))).isFalse(); + assertThat(matcher.matches(withClazzNameWithPackage("com.pkg.SampleService1$$EnhancerBySpringCGLIB$$1cdd7e7b$$FastClassBySpringCGLIB$$6b3308cd"))).isFalse(); + } + } + @Nested @DisplayName("if package is set") class PackageNameTest { diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java index 814546cd..76537754 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistryTest.java @@ -38,19 +38,6 @@ void cached() { assertThat(sut.getHash(signature)).isEqualTo(expected); } } - - @Nested - @DisplayName("if method is generated by SpringCGLIB") - class CglibTest { - String cglibSignature = "TestClass$$EnhancerBySpringCGLIB$$hash.testMethod()"; - - @Test - @DisplayName("it returns empty string") - void returnNull() { - assertThat(sut.getHash(cglibSignature)) - .isEqualTo(""); - } - } } @Test From 06da697e71cf7a23c5d0523825c9f2cc7bf86a6d Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 5 Oct 2024 21:25:51 +1000 Subject: [PATCH 09/17] Remove worker thread InvocationRegistry --- .../collecting/InvocationRegistry.java | 34 ++----------------- 1 file changed, 3 insertions(+), 31 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java index b921fc86..f5ba3f01 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java @@ -1,15 +1,12 @@ package com.navercorp.scavenger.javaagent.collecting; -import java.util.HashSet; import java.util.Set; -import java.util.concurrent.BlockingQueue; -import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import lombok.extern.java.Log; import com.navercorp.scavenger.javaagent.model.Config; -import com.navercorp.scavenger.javaagent.scheduling.ScavengerThreadFactory; import com.navercorp.scavenger.model.InvocationDataPublication; @Log @@ -17,37 +14,12 @@ public class InvocationRegistry { private static final int FRONT_BUFFER_INDEX = 0; private static final int BACK_BUFFER_INDEX = 1; - private final Set[] invocations; - private final BlockingQueue queue = new LinkedBlockingQueue<>(); + private final Set[] invocations = new Set[]{ConcurrentHashMap.newKeySet(), ConcurrentHashMap.newKeySet()}; private volatile int currentInvocationIndex = FRONT_BUFFER_INDEX; private long recordingIntervalStartedAtMillis = System.currentTimeMillis(); - public InvocationRegistry() { - //noinspection unchecked - this.invocations = new Set[] {new HashSet<>(), new HashSet<>()}; - Thread worker = ScavengerThreadFactory.builder() - .name("registry") - .build() - .newThread(new Thread(this::workerTask)); - worker.start(); - } - - void workerTask() { - while (!Thread.currentThread().isInterrupted()) { - try { - invocations[currentInvocationIndex].add(queue.take()); - } catch (InterruptedException e) { - log.fine("[scavenger] Interrupted"); - Thread.currentThread().interrupt(); - return; - } - } - } - public void register(String hash) { - if (!invocations[currentInvocationIndex].contains(hash)) { - queue.add(hash); - } + invocations[currentInvocationIndex].add(hash); } private synchronized void toggleInvocationsIndex() { From 779f74eb0b02d692ae1cd6f8dfc7d7464cb80fdb Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 9 Oct 2024 22:00:42 +1100 Subject: [PATCH 10/17] Rework InvocationRegistry to be faster on buffer reset --- .../collecting/InvocationRegistry.java | 59 ++++++++----------- .../collecting/InvocationRegistryTest.java | 3 - 2 files changed, 24 insertions(+), 38 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java index f5ba3f01..32255636 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java @@ -1,8 +1,9 @@ package com.navercorp.scavenger.javaagent.collecting; -import java.util.Set; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; import java.util.concurrent.ConcurrentHashMap; -import java.util.stream.Collectors; import lombok.extern.java.Log; @@ -11,53 +12,41 @@ @Log public class InvocationRegistry { - private static final int FRONT_BUFFER_INDEX = 0; - private static final int BACK_BUFFER_INDEX = 1; - private final Set[] invocations = new Set[]{ConcurrentHashMap.newKeySet(), ConcurrentHashMap.newKeySet()}; - private volatile int currentInvocationIndex = FRONT_BUFFER_INDEX; + private static class BooleanHolder { + private volatile boolean value = true; + } + + private final ConcurrentHashMap invocations = new ConcurrentHashMap<>(); private long recordingIntervalStartedAtMillis = System.currentTimeMillis(); public void register(String hash) { - invocations[currentInvocationIndex].add(hash); - } - - private synchronized void toggleInvocationsIndex() { - recordingIntervalStartedAtMillis = System.currentTimeMillis(); - currentInvocationIndex = currentInvocationIndex == FRONT_BUFFER_INDEX ? BACK_BUFFER_INDEX : FRONT_BUFFER_INDEX; + BooleanHolder holder = invocations.computeIfAbsent(hash, k -> new BooleanHolder()); + if (!holder.value) { + holder.value = true; + } } public InvocationDataPublication getPublication(Config config, String codeBaseFingerprint) { + List dataEntries = new ArrayList<>(); + for (Map.Entry entry : invocations.entrySet()) { + if (entry.getValue().value) { + entry.getValue().value = false; + dataEntries.add(InvocationDataPublication.InvocationDataEntry.newBuilder() + .setHash(entry.getKey()) + .build()); + } + } long oldRecordingIntervalStartedAtMillis = recordingIntervalStartedAtMillis; - int oldIndex = currentInvocationIndex; - - toggleInvocationsIndex(); - - try { - Thread.sleep(10L); - - return InvocationDataPublication.newBuilder() + recordingIntervalStartedAtMillis = System.currentTimeMillis(); + return InvocationDataPublication.newBuilder() .setCommonData( config.buildCommonPublicationData().toBuilder() .setCodeBaseFingerprint(codeBaseFingerprint) .build() ) - .addAllEntry( - invocations[oldIndex].stream() - .map(it -> - InvocationDataPublication.InvocationDataEntry.newBuilder() - .setHash(it) - .build() - ).collect(Collectors.toList()) - ) + .addAllEntry(dataEntries) .setRecordingIntervalStartedAtMillis(oldRecordingIntervalStartedAtMillis) .build(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } finally { - invocations[oldIndex].clear(); - } - - return null; } } diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistryTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistryTest.java index 29ccc2d1..5211095f 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistryTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistryTest.java @@ -6,7 +6,6 @@ import java.util.Properties; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; @@ -14,9 +13,7 @@ import com.navercorp.scavenger.javaagent.model.Config; import com.navercorp.scavenger.model.InvocationDataPublication; -// FIXME flaky test @Nested -@Disabled @DisplayName("InvocationRegistry class") public class InvocationRegistryTest { InvocationRegistry sut; From 0f02f4dcbb3c89e70e70ab713eb7813d40fa2f45 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Sat, 4 Jan 2025 00:22:04 +1100 Subject: [PATCH 11/17] ConcurrentHashMap optimisation --- .../scavenger/javaagent/collecting/InvocationRegistry.java | 6 ++++-- .../scavenger/javaagent/collecting/MethodRegistry.java | 7 ++++++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java index 32255636..5f0c63a0 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationRegistry.java @@ -21,8 +21,10 @@ private static class BooleanHolder { private long recordingIntervalStartedAtMillis = System.currentTimeMillis(); public void register(String hash) { - BooleanHolder holder = invocations.computeIfAbsent(hash, k -> new BooleanHolder()); - if (!holder.value) { + BooleanHolder holder = invocations.get(hash); + if (holder == null) { + invocations.put(hash, new BooleanHolder()); + } else if (!holder.value) { holder.value = true; } } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java index 64d9aec9..513e7b07 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java @@ -20,7 +20,12 @@ public MethodRegistry(boolean isLegacyCompatibilityMode) { } public String getHash(String byteBuddySignature) { - return byteBuddySignatureToHash.computeIfAbsent(byteBuddySignature, this::generateHash); + String hash = byteBuddySignatureToHash.get(byteBuddySignature); + if (hash == null) { + hash = generateHash(byteBuddySignature); + byteBuddySignatureToHash.put(byteBuddySignature, hash); + } + return hash; } private String generateHash(String byteBuddySignature) { From e43688a85face0aa36bca2a9db3912f5339d644c Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 10 Oct 2024 19:00:50 +1100 Subject: [PATCH 12/17] Upgrade agent tests to Java 21 --- scavenger-agent-java/build.gradle.kts | 13 ++++++++++--- .../integrationTest/javaagent/BenchmarkTest.java | 2 -- .../javaagent/scheduling/SchedulerTest.java | 13 +++---------- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/scavenger-agent-java/build.gradle.kts b/scavenger-agent-java/build.gradle.kts index 5c3fb730..fc71e120 100644 --- a/scavenger-agent-java/build.gradle.kts +++ b/scavenger-agent-java/build.gradle.kts @@ -11,13 +11,18 @@ plugins { java { toolchain { - languageVersion.set(JavaLanguageVersion.of(8)) + languageVersion.set(JavaLanguageVersion.of(21)) } - withJavadocJar() withSourcesJar() } +tasks.withType().matching { + it.name in setOf("compileJava", "compileIntegrationTestJava") +}.configureEach { + options.release = 8 +} + tasks.withType { archiveFileName.set("${project.name}-${project.version}.jar") @@ -67,9 +72,11 @@ dependencies { implementation("io.grpc:grpc-okhttp:${property("grpcVersion")}") testImplementation(platform("org.junit:junit-bom:5.8.2")) + testImplementation(platform("org.mockito:mockito-bom:5.13.0")) testImplementation("org.junit.jupiter:junit-jupiter") testImplementation("org.assertj:assertj-core:3.22.0") - testImplementation("org.mockito:mockito-inline:4.3.1") + testImplementation("org.mockito:mockito-core") + testImplementation("org.mockito:mockito-junit-jupiter") } testSets { diff --git a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java index e2287692..898c3af7 100644 --- a/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java +++ b/scavenger-agent-java/src/integrationTest/java/integrationTest/javaagent/BenchmarkTest.java @@ -2,13 +2,11 @@ import integrationTest.support.AgentBenchmarkExtension; import integrationTest.support.AgentRunner; -import lombok.extern.java.Log; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.TestTemplate; import org.junit.jupiter.api.extension.ExtendWith; -@Log @Disabled("Comment/remove this annotation to run benchmarks") @ExtendWith(AgentBenchmarkExtension.class) public class BenchmarkTest { diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java index 8e50b0bf..0fc1dd73 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java @@ -6,13 +6,12 @@ import java.util.Collections; import java.util.Properties; -import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; import lombok.SneakyThrows; @@ -26,7 +25,9 @@ import com.navercorp.scavenger.model.CommonPublicationData; import com.navercorp.scavenger.model.GetConfigResponse; import com.navercorp.scavenger.model.InvocationDataPublication; +import org.mockito.junit.jupiter.MockitoExtension; +@ExtendWith(MockitoExtension.class) @Nested @DisplayName("Scheduler class") public class SchedulerTest { @@ -64,8 +65,6 @@ public class SchedulerTest { @Mock private Publisher publisher; - private AutoCloseable autoCloseable; - @Mock private CodeBaseScanner codeBaseScannerMock; @@ -79,7 +78,6 @@ private InvocationDataPublication.InvocationDataEntry newInvocationDataEntry(Str @SneakyThrows @BeforeEach public void setUp() { - autoCloseable = MockitoAnnotations.openMocks(this); when(codeBaseScannerMock.scan()) .thenReturn( new CodeBase(Collections.singletonList(Method.createTestMethod()), "fingerprint") @@ -88,11 +86,6 @@ public void setUp() { sut = new Scheduler(new Config(new Properties()), publisher, codeBaseScannerMock); } - @AfterEach - public void tearDown() throws Exception { - autoCloseable.close(); - } - @Nested @DisplayName("run method") class RunMethod { From d16fb95a7888de4231f324585cb8f3360d7050fa Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 10 Oct 2024 19:40:55 +1100 Subject: [PATCH 13/17] Rework InvocationTracker injection --- .../scavenger/javaagent/ScavengerAgent.java | 15 +++-- .../collecting/InvocationTracker.java | 30 +++++---- .../javaagent/scheduling/Scheduler.java | 11 +-- .../javaagent/scheduling/SchedulerTest.java | 67 +++++++++---------- 4 files changed, 66 insertions(+), 57 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/ScavengerAgent.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/ScavengerAgent.java index 53220755..71e168ff 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/ScavengerAgent.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/ScavengerAgent.java @@ -7,10 +7,10 @@ import lombok.extern.java.Log; -import com.navercorp.scavenger.javaagent.collecting.CodeBaseScanner; +import com.navercorp.scavenger.javaagent.collecting.InvocationRegistry; import com.navercorp.scavenger.javaagent.collecting.InvocationTracker; +import com.navercorp.scavenger.javaagent.collecting.MethodRegistry; import com.navercorp.scavenger.javaagent.collecting.ScavengerBanner; -import com.navercorp.scavenger.javaagent.model.CodeBase; import com.navercorp.scavenger.javaagent.model.Config; import com.navercorp.scavenger.javaagent.scheduling.Scheduler; import com.navercorp.scavenger.javaagent.scheduling.ShutdownHook; @@ -39,17 +39,22 @@ public static void premain(String args, Instrumentation inst) { new ScavengerBanner(config).printBanner(System.out); - Scheduler scheduler = new Scheduler(config); + InvocationRegistry invocationRegistry = new InvocationRegistry(); + InvocationTracker tracker = new InvocationTracker( + invocationRegistry, + new MethodRegistry(config.isLegacyCompatibilityMode()), + config.isDebugMode()); + Scheduler scheduler = new Scheduler(invocationRegistry, config); + if (!config.isAsyncCodeBaseScanMode()) { boolean scanSuccessful = scheduler.scanCodeBase(); - if (!scanSuccessful) { log.warning("[scavenger] scavenger is disabled"); return; } } - InvocationTracker.installAdvice(inst, config); + tracker.installAdvice(inst, config); scheduler.start(); Runtime.getRuntime().addShutdownHook(new ShutdownHook(scheduler)); } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java index 21387e4c..1466f2c0 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/InvocationTracker.java @@ -10,24 +10,28 @@ import net.bytebuddy.dynamic.scaffold.TypeValidation; import net.bytebuddy.utility.JavaModule; -import lombok.Getter; -import lombok.Setter; import lombok.extern.java.Log; import com.navercorp.scavenger.javaagent.model.Config; @Log public class InvocationTracker { - @Getter - @Setter - private static InvocationRegistry invocationRegistry = new InvocationRegistry(); - private static MethodRegistry methodRegistry; - private static boolean isDebugMode; + private final InvocationRegistry invocationRegistry; + private final MethodRegistry methodRegistry; + private final boolean isDebugMode; - public static void installAdvice(Instrumentation inst, Config config) { - methodRegistry = new MethodRegistry(config.isLegacyCompatibilityMode()); - isDebugMode = config.isDebugMode(); + private static InvocationTracker INSTANCE; + public InvocationTracker(InvocationRegistry invocationRegistry, + MethodRegistry methodRegistry, + boolean isDebugMode) { + this.invocationRegistry = invocationRegistry; + this.methodRegistry = methodRegistry; + this.isDebugMode = isDebugMode; + INSTANCE = this; + } + + public void installAdvice(Instrumentation inst, Config config) { ElementMatcherBuilder matcherBuilder = new ElementMatcherBuilder(config); Advice advice = Advice.to(InvocationTracker.class); AgentBuilder transform = new AgentBuilder.Default(new ByteBuddy().with(TypeValidation.DISABLED)) @@ -59,10 +63,10 @@ static void onInvocation(@Advice.Origin String signature) { } public static void hashAndRegister(String signature) { - String hash = methodRegistry.getHash(signature); - if (isDebugMode) { + String hash = INSTANCE.methodRegistry.getHash(signature); + if (INSTANCE.isDebugMode) { log.info("[scavenger] method " + signature + " is invoked - " + hash); } - invocationRegistry.register(hash); + INSTANCE.invocationRegistry.register(hash); } } diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java index 60d5cf55..eb70069e 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java @@ -5,6 +5,7 @@ import java.util.concurrent.TimeUnit; import java.util.logging.Level; +import com.navercorp.scavenger.javaagent.collecting.InvocationRegistry; import io.grpc.Status; import io.grpc.StatusRuntimeException; import lombok.extern.java.Log; @@ -20,6 +21,7 @@ @Log public class Scheduler implements Runnable { + private final InvocationRegistry invocationRegistry; private final Config config; private final Publisher publisher; private final CodeBaseScanner codeBaseScanner; @@ -38,11 +40,12 @@ public class Scheduler implements Runnable { private InvocationDataPublication invocationDataPublication; private String codeBaseFingerprint; - public Scheduler(Config config) { - this(config, new Publisher(config), new CodeBaseScanner(config)); + public Scheduler(InvocationRegistry invocationRegistry, Config config) { + this(invocationRegistry, config, new Publisher(config), new CodeBaseScanner(config)); } - public Scheduler(Config config, Publisher publisher, CodeBaseScanner codeBaseScanner) { + public Scheduler(InvocationRegistry invocationRegistry, Config config, Publisher publisher, CodeBaseScanner codeBaseScanner) { + this.invocationRegistry = invocationRegistry; this.config = config; this.codeBaseScanner = codeBaseScanner; this.publisher = publisher; @@ -199,7 +202,7 @@ public void publishInvocationDataIfNeeded() { if (invocationDataPublisherState.isDueTime() && dynamicConfig != null && isCodeBasePublished) { try { if (invocationDataPublication == null) { - invocationDataPublication = InvocationTracker.getInvocationRegistry() + invocationDataPublication = invocationRegistry .getPublication( config, codeBaseFingerprint diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java index 0fc1dd73..3fcfa284 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/scheduling/SchedulerTest.java @@ -1,9 +1,18 @@ package com.navercorp.scavenger.javaagent.scheduling; -import static org.mockito.Mockito.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.atMostOnce; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.verifyNoMoreInteractions; +import static org.mockito.Mockito.when; import java.io.IOException; -import java.util.Collections; +import java.util.List; import java.util.Properties; import org.junit.jupiter.api.BeforeEach; @@ -13,11 +22,8 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; -import lombok.SneakyThrows; - import com.navercorp.scavenger.javaagent.collecting.CodeBaseScanner; import com.navercorp.scavenger.javaagent.collecting.InvocationRegistry; -import com.navercorp.scavenger.javaagent.collecting.InvocationTracker; import com.navercorp.scavenger.javaagent.model.CodeBase; import com.navercorp.scavenger.javaagent.model.Config; import com.navercorp.scavenger.javaagent.model.Method; @@ -62,28 +68,32 @@ public class SchedulerTest { .setInvocationDataPublisherRetryIntervalSeconds(0) .build(); + @Mock + private InvocationRegistry invocationRegistry; + @Mock private Publisher publisher; @Mock private CodeBaseScanner codeBaseScannerMock; - @SuppressWarnings("SameParameterValue") - private InvocationDataPublication.InvocationDataEntry newInvocationDataEntry(String hash) { - return InvocationDataPublication.InvocationDataEntry.newBuilder() - .setHash(hash) - .build(); + @BeforeEach + public void setUp() throws Exception { + lenient().when(codeBaseScannerMock.scan()) + .thenReturn(new CodeBase(List.of(Method.createTestMethod()), "fingerprint")); + sut = new Scheduler(invocationRegistry, new Config(new Properties()), publisher, codeBaseScannerMock); } - @SneakyThrows - @BeforeEach - public void setUp() { - when(codeBaseScannerMock.scan()) + private void mockInvocationRegistered() { + when(invocationRegistry.getPublication(any(), anyString())) .thenReturn( - new CodeBase(Collections.singletonList(Method.createTestMethod()), "fingerprint") + InvocationDataPublication.newBuilder() + .setCommonData(sampleCommonData) + .addEntry(InvocationDataPublication.InvocationDataEntry.newBuilder() + .setHash("hash") + .build()) + .build() ); - - sut = new Scheduler(new Config(new Properties()), publisher, codeBaseScannerMock); } @Nested @@ -97,7 +107,7 @@ class PollSucceedInvocationRegistered { @BeforeEach public void setUpAndRun() { when(publisher.pollDynamicConfig()).thenReturn(configResponse); - InvocationTracker.getInvocationRegistry().register("hash"); + mockInvocationRegistered(); sut.run(); } @@ -126,7 +136,7 @@ void publishInvocationData() { class NoInvocationTest { @BeforeEach - public void setUpAndRun() throws IOException { + public void setUpAndRun() { when(publisher.pollDynamicConfig()).thenReturn(configResponse); sut.run(); } @@ -206,17 +216,13 @@ void publishThreeTimes() { @Nested @DisplayName("if invocation publish failed") class InvocationPublishTest { - InvocationRegistry registry; @BeforeEach public void setUpAndRunThreeTimes() throws IOException { - registry = spy(InvocationRegistry.class); - when(publisher.pollDynamicConfig()) .thenReturn(configResponse); - InvocationTracker.setInvocationRegistry(registry); - registry.register("hash"); + mockInvocationRegistered(); doThrow(new RuntimeException()) .when(publisher) @@ -231,7 +237,7 @@ public void setUpAndRunThreeTimes() throws IOException { @Test @DisplayName("it runs getPublication only once") void getOnce() { - verify(registry, atMostOnce()).getPublication(any(), anyString()); + verify(invocationRegistry, atMostOnce()).getPublication(any(), anyString()); } @Test @@ -268,17 +274,8 @@ class NormalConditionTest { @BeforeEach public void runsAndShutdown() throws IOException { - InvocationRegistry registry = mock(InvocationRegistry.class); - InvocationTracker.setInvocationRegistry(registry); - + mockInvocationRegistered(); when(publisher.pollDynamicConfig()).thenReturn(configResponse); - when(registry.getPublication(any(), anyString())) - .thenReturn( - InvocationDataPublication.newBuilder() - .setCommonData(sampleCommonData) - .addEntry(newInvocationDataEntry("hash")) - .build() - ); sut.run(); sut.shutdown(); From 135a84ebfa2df61886333083ab678ea6dbb3abc8 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Thu, 10 Oct 2024 22:04:35 +1100 Subject: [PATCH 14/17] Do not flood logs if collector goes down --- .../navercorp/scavenger/javaagent/scheduling/Scheduler.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java index eb70069e..354976e3 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/scheduling/Scheduler.java @@ -213,7 +213,11 @@ public void publishInvocationDataIfNeeded() { invocationDataPublication = null; invocationDataPublisherState.scheduleNext(); } catch (Exception e) { - log.log(Level.SEVERE, "[scavenger] invocation data publish failed", e); + if (e instanceof StatusRuntimeException && ((StatusRuntimeException) e).getStatus().equals(Status.UNAVAILABLE)) { + log.log(Level.SEVERE, "[scavenger] invocation data publish failed (server unavailable)"); + } else { + log.log(Level.SEVERE, "[scavenger] invocation data publish failed", e); + } invocationDataPublisherState.scheduleRetry(); } } From 834fc49db9f505bda85aec702eebec38fd23f3c5 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 2 Oct 2024 19:29:46 +1000 Subject: [PATCH 15/17] Refactor HashGenerator to support default hashing implementation --- .../javaagent/collecting/MethodRegistry.java | 4 +- .../scavenger/javaagent/model/Method.java | 4 +- .../scavenger/javaagent/model/MethodTest.java | 6 +-- .../navercorp/scavenger/model/Publication.kt | 6 +-- .../scavenger/model/PublicationTest.kt | 20 +++---- .../scavenger/util/HashGenerator.java | 53 +++++++++++-------- 6 files changed, 50 insertions(+), 43 deletions(-) diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java index 513e7b07..0f6d448b 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/collecting/MethodRegistry.java @@ -6,7 +6,7 @@ import lombok.Getter; import lombok.extern.java.Log; -import com.navercorp.scavenger.util.HashGenerator; +import com.navercorp.scavenger.util.HashGenerator.DefaultHash; @Log public class MethodRegistry { @@ -33,7 +33,7 @@ private String generateHash(String byteBuddySignature) { if (isLegacyCompatibilityMode) { signature = signature.replace('$', '.').replace(",", ", "); } - return HashGenerator.Md5.from(signature); + return DefaultHash.from(signature); } static String extractSignature(String byteBuddySignature) { diff --git a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/model/Method.java b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/model/Method.java index 086d5608..54960664 100644 --- a/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/model/Method.java +++ b/scavenger-agent-java/src/main/java/com/navercorp/scavenger/javaagent/model/Method.java @@ -14,7 +14,7 @@ import lombok.Value; import com.navercorp.scavenger.model.CodeBasePublication; -import com.navercorp.scavenger.util.HashGenerator; +import com.navercorp.scavenger.util.HashGenerator.DefaultHash; @Value public class Method { @@ -111,7 +111,7 @@ public CodeBasePublication.CodeBaseEntry toCodeBaseEntry() { .setModifiers(defaultIfNull(this.modifiers, "")) .setPackageName(defaultIfNull(packageName, "")) .setParameterTypes(defaultIfNull(this.parameterTypes, "")) - .setSignatureHash(HashGenerator.Md5.from(signature)) + .setSignatureHash(DefaultHash.from(signature)) .build(); } diff --git a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/model/MethodTest.java b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/model/MethodTest.java index 01e1711a..989820ab 100644 --- a/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/model/MethodTest.java +++ b/scavenger-agent-java/src/test/java/com/navercorp/scavenger/javaagent/model/MethodTest.java @@ -4,7 +4,7 @@ import org.junit.jupiter.api.Test; -import com.navercorp.scavenger.util.HashGenerator; +import com.navercorp.scavenger.util.HashGenerator.DefaultHash; class MethodTest { @Test @@ -19,7 +19,7 @@ void testMethodCreation() { assertThat(it.getModifiers()).isEqualTo("static"); assertThat(it.getPackageName()).isEqualTo("packageName"); assertThat(it.getSignature()).isEqualTo("name(int a, int wow)"); - assertThat(it.getSignatureHash()).isEqualTo(HashGenerator.Md5.from("name(int a, int wow)")); + assertThat(it.getSignatureHash()).isEqualTo(DefaultHash.from("name(int a, int wow)")); }); Method method2 = new Method(null, Visibility.PRIVATE, null, false, false, "", "int a,int wow", null); @@ -30,7 +30,7 @@ void testMethodCreation() { assertThat(it.getModifiers()).isEqualTo(""); assertThat(it.getPackageName()).isEqualTo(""); assertThat(it.getSignature()).isEqualTo(""); - assertThat(it.getSignatureHash()).isEqualTo(HashGenerator.Md5.from("")); + assertThat(it.getSignatureHash()).isEqualTo(DefaultHash.from("")); }); } diff --git a/scavenger-collector/src/main/kotlin/com/navercorp/scavenger/model/Publication.kt b/scavenger-collector/src/main/kotlin/com/navercorp/scavenger/model/Publication.kt index 87e5969f..3e724151 100644 --- a/scavenger-collector/src/main/kotlin/com/navercorp/scavenger/model/Publication.kt +++ b/scavenger-collector/src/main/kotlin/com/navercorp/scavenger/model/Publication.kt @@ -5,7 +5,7 @@ import com.navercorp.scavenger.dto.CommonImportDto import com.navercorp.scavenger.dto.CommonImportResultDto import com.navercorp.scavenger.dto.InvocationImportDto import com.navercorp.scavenger.exception.UnknownPublicationException -import com.navercorp.scavenger.util.HashGenerator +import com.navercorp.scavenger.util.HashGenerator.DefaultHash import io.codekvast.javaagent.model.v4.CodeBasePublication4 import io.codekvast.javaagent.model.v4.CommonPublicationData4 import io.codekvast.javaagent.model.v4.InvocationDataPublication4 @@ -117,7 +117,7 @@ sealed class LegacyPublication private constructor(val commonData: CommonPublica modifiers = it.methodSignature.modifiers, packageName = it.methodSignature.packageName, parameterTypes = it.methodSignature.parameterTypes, - signatureHash = HashGenerator.Md5.from(it.signature) + signatureHash = DefaultHash.from(it.signature) ) }.sortedBy { it.signatureHash } @@ -141,7 +141,7 @@ sealed class LegacyPublication private constructor(val commonData: CommonPublica environmentId = environmentId, invocations = pub.invocations .filterNot { syntheticSignaturePattern.matcher(it).matches() } - .map { HashGenerator.Md5.from(it) } + .map { DefaultHash.from(it) } .sorted(), invokedAtMillis = pub.recordingIntervalStartedAtMillis, ) diff --git a/scavenger-collector/src/test/kotlin/com/navercorp/scavenger/model/PublicationTest.kt b/scavenger-collector/src/test/kotlin/com/navercorp/scavenger/model/PublicationTest.kt index 8e9cd21a..1c9c75d5 100644 --- a/scavenger-collector/src/test/kotlin/com/navercorp/scavenger/model/PublicationTest.kt +++ b/scavenger-collector/src/test/kotlin/com/navercorp/scavenger/model/PublicationTest.kt @@ -2,7 +2,7 @@ package com.navercorp.scavenger.model import com.navercorp.scavenger.dto.CommonImportResultDto import com.navercorp.scavenger.model.CodeBasePublication.CodeBaseEntry -import com.navercorp.scavenger.util.HashGenerator.Md5 +import com.navercorp.scavenger.util.HashGenerator.DefaultHash import com.navercorp.scavenger.util.SamplePublications import io.codekvast.javaagent.model.v4.CodeBaseEntry4 import io.codekvast.javaagent.model.v4.CodeBasePublication4 @@ -72,14 +72,14 @@ class PublicationTest { ).getCodeBaseImportDto(commonImportResultDto).entries ).hasSize(2) .extracting("signatureHash") - .isEqualTo(listOf(Md5.from(targetSignature), Md5.from(CodeBaseEntry4.sampleCodeBaseEntry().signature)).sorted()) + .isEqualTo(listOf(DefaultHash.from(targetSignature), DefaultHash.from(CodeBaseEntry4.sampleCodeBaseEntry().signature)).sorted()) } } @Nested @DisplayName("if invocation data contains SpringCGLIB generated method") inner class Invocation { - private val cglibHash = Md5.from(cglibSignature) + private val cglibHash = DefaultHash.from(cglibSignature) @Test @DisplayName("it ignores it") @@ -117,7 +117,7 @@ class PublicationTest { ) .build() ).getInvocationImportDto(commonImportResultDto).invocations - ).isEqualTo(listOf(Md5.from(targetSignature), Md5.from("signature()")).sorted()) + ).isEqualTo(listOf(DefaultHash.from(targetSignature), DefaultHash.from("signature()")).sorted()) } } } @@ -129,10 +129,10 @@ class PublicationTest { private val codeBaseEntries = listOf( CodeBaseEntry.newBuilder() - .setSignatureHash((Md5.from("TestClass.method()"))) + .setSignatureHash(DefaultHash.from("TestClass.method()")) .build(), CodeBaseEntry.newBuilder() - .setSignatureHash((Md5.from("signature()"))) + .setSignatureHash(DefaultHash.from("signature()")) .build() ) @@ -149,7 +149,7 @@ class PublicationTest { .getCodeBaseImportDto(commonImportResultDto).entries ) .extracting("signatureHash") - .isEqualTo(listOf(Md5.from("TestClass.method()"), Md5.from("signature()")).sorted()) + .isEqualTo(listOf(DefaultHash.from("TestClass.method()"), DefaultHash.from("signature()")).sorted()) } @Test @@ -161,17 +161,17 @@ class PublicationTest { .setCommonData(SamplePublications.commonPublicationData) .addEntry( InvocationDataPublication.InvocationDataEntry.newBuilder() - .setHash(Md5.from("signature()")) + .setHash(DefaultHash.from("signature()")) ) .addEntry( InvocationDataPublication.InvocationDataEntry.newBuilder() - .setHash(Md5.from("TestClass.method()")) + .setHash(DefaultHash.from("TestClass.method()")) ) .setRecordingIntervalStartedAtMillis(0) .build() ).getInvocationImportDto(commonImportResultDto).invocations ) - .isEqualTo(listOf(Md5.from("TestClass.method()"), Md5.from("signature()")).sorted()) + .isEqualTo(listOf(DefaultHash.from("TestClass.method()"), DefaultHash.from("signature()")).sorted()) } } } diff --git a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java index f98e4f38..5c01d239 100644 --- a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java +++ b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java @@ -3,35 +3,42 @@ import java.math.BigInteger; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; -import java.security.NoSuchAlgorithmException; +import java.util.concurrent.Callable; public class HashGenerator { - public static class Sha256 { + + public static class DefaultHash { public static String from(String signature) { - try { - MessageDigest md = MessageDigest.getInstance("SHA-256"); - md.update(signature.getBytes(StandardCharsets.UTF_8)); - return String.format("%x", new BigInteger(1, md.digest())); - } catch (NoSuchAlgorithmException ignore) { - // ignore - return null; - } + return Md5.from(signature); } } - public static class Md5 { - public static String from(String signature) { - try { - if (signature == null) { - return null; - } - MessageDigest md = MessageDigest.getInstance("MD5"); - md.update(signature.getBytes(StandardCharsets.UTF_8)); - return String.format("%x", new BigInteger(1, md.digest())); - } catch (NoSuchAlgorithmException ignore) { - // ignore - return null; - } + private static class Sha256 { + private static String from(String signature) { + MessageDigest md = callWithCheckedExceptionWrapping(() -> MessageDigest.getInstance("SHA-256")); + md.update(signature.getBytes(StandardCharsets.UTF_8)); + return String.format("%x", new BigInteger(1, md.digest())); + } + } + + private static class Md5 { + private static String from(String signature) { + MessageDigest md = callWithCheckedExceptionWrapping(() -> MessageDigest.getInstance("MD5")); + md.update(signature.getBytes(StandardCharsets.UTF_8)); + return String.format("%x", new BigInteger(1, md.digest())); + } + } + + /** + * Utility method to call a {@link Callable} and rethrow any exceptions as unchecked. + */ + private static T callWithCheckedExceptionWrapping(Callable callable) { + try { + return callable.call(); + } catch (RuntimeException e) { + throw e; + } catch (Exception e) { + throw new RuntimeException(e); } } } From 88fe8c3a647edc9f486d698a1e6bab1137a78a58 Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Wed, 9 Oct 2024 22:00:05 +1100 Subject: [PATCH 16/17] Implement MurmurHash3 --- scavenger-model/build.gradle.kts | 8 +++++++- .../com/navercorp/scavenger/util/HashGenerator.java | 11 ++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/scavenger-model/build.gradle.kts b/scavenger-model/build.gradle.kts index 33eef450..b040a3b4 100644 --- a/scavenger-model/build.gradle.kts +++ b/scavenger-model/build.gradle.kts @@ -1,4 +1,9 @@ -import com.google.protobuf.gradle.* +import com.google.protobuf.gradle.builtins +import com.google.protobuf.gradle.generateProtoTasks +import com.google.protobuf.gradle.id +import com.google.protobuf.gradle.plugins +import com.google.protobuf.gradle.protobuf +import com.google.protobuf.gradle.protoc plugins { java @@ -17,6 +22,7 @@ dependencies { implementation("io.grpc:grpc-kotlin-stub:${property("grpcKotlinVersion")}") implementation("io.grpc:grpc-protobuf:${property("grpcVersion")}") implementation("javax.annotation:javax.annotation-api:1.3.2") + implementation("commons-codec:commons-codec:1.17.1") } kotlin { diff --git a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java index 5c01d239..f09c1cb7 100644 --- a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java +++ b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java @@ -1,5 +1,7 @@ package com.navercorp.scavenger.util; +import org.apache.commons.codec.digest.MurmurHash3; + import java.math.BigInteger; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; @@ -9,7 +11,14 @@ public class HashGenerator { public static class DefaultHash { public static String from(String signature) { - return Md5.from(signature); + return Murmur.from(signature); + } + } + + private static class Murmur { + private static String from(String signature) { + long[] x64hash = MurmurHash3.hash128x64(signature.getBytes(StandardCharsets.UTF_8)); + return Long.toHexString(x64hash[0]) + Long.toHexString(x64hash[1]); } } From d15a68cf697a739326748dc22c2aff4415e72fbc Mon Sep 17 00:00:00 2001 From: Kusal Kithul-Godage Date: Tue, 24 Dec 2024 17:05:25 +1100 Subject: [PATCH 17/17] Implement configurable algorithm with sys prop --- .../scavenger/util/HashGenerator.java | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java index f09c1cb7..1e693b56 100644 --- a/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java +++ b/scavenger-model/src/main/java/com/navercorp/scavenger/util/HashGenerator.java @@ -6,12 +6,35 @@ import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.util.concurrent.Callable; +import java.util.function.Function; public class HashGenerator { + public enum HashAlgorithm { + + MURMUR(Murmur::from), + SHA256(Sha256::from), + MD5(Md5::from); + + private final Function hashFunction; + + HashAlgorithm(Function hashFunction) { + this.hashFunction = hashFunction; + } + + public String hash(String signature) { + return hashFunction.apply(signature); + } + } + public static class DefaultHash { + private static final String SELECTED_ALGORITHM_PROP = "hash.algorithm"; + private static final HashAlgorithm DEFAULT_ALGORITHM = HashAlgorithm.MD5; + private static final HashAlgorithm SELECTED_ALGORITHM = + HashAlgorithm.valueOf(System.getProperty(SELECTED_ALGORITHM_PROP, DEFAULT_ALGORITHM.name()).toUpperCase()); + public static String from(String signature) { - return Murmur.from(signature); + return SELECTED_ALGORITHM.hash(signature); } }