From 57eb99c8cd0a133df1ad92bb7ae2dfe394f77b63 Mon Sep 17 00:00:00 2001 From: John Bedell Date: Tue, 9 Jan 2024 09:33:39 -0500 Subject: [PATCH 1/3] Swap the ProcessPointCut over to a weaver instrumentation module to better handle cases where it is used in a multi-threaded environment. --- instrumentation/java.process/build.gradle | 20 +++++++++++ .../java/lang/Process_Instrumentation.java | 20 +++++++++++ .../newrelic/agent/TransactionActivity.java | 25 +++++++++++++ .../pointcuts/ProcessPointCut.java | 36 ------------------- settings.gradle | 1 + 5 files changed, 66 insertions(+), 36 deletions(-) create mode 100644 instrumentation/java.process/build.gradle create mode 100644 instrumentation/java.process/src/main/java/java/lang/Process_Instrumentation.java delete mode 100644 newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/pointcuts/ProcessPointCut.java diff --git a/instrumentation/java.process/build.gradle b/instrumentation/java.process/build.gradle new file mode 100644 index 0000000000..b7c3b16ba1 --- /dev/null +++ b/instrumentation/java.process/build.gradle @@ -0,0 +1,20 @@ +dependencies { + implementation(project(":agent-bridge")) +} + +// This instrumentation module should not use the bootstrap classpath + + +jar { + manifest { attributes 'Implementation-Title': 'com.newrelic.instrumentation.java.process' } +} + +verifyInstrumentation { + verifyClasspath = false // We don't want to verify classpath since these are JDK classes +} + +site { + title 'Java Process' + type 'Other' + versionOverride '[8,)' +} \ No newline at end of file diff --git a/instrumentation/java.process/src/main/java/java/lang/Process_Instrumentation.java b/instrumentation/java.process/src/main/java/java/lang/Process_Instrumentation.java new file mode 100644 index 0000000000..7b2f87dcc0 --- /dev/null +++ b/instrumentation/java.process/src/main/java/java/lang/Process_Instrumentation.java @@ -0,0 +1,20 @@ +package java.lang; + +import com.newrelic.agent.bridge.AgentBridge; +import com.newrelic.agent.bridge.Transaction; +import com.newrelic.api.agent.Trace; +import com.newrelic.api.agent.weaver.MatchType; +import com.newrelic.api.agent.weaver.Weave; +import com.newrelic.api.agent.weaver.Weaver; + +@Weave(type = MatchType.Interface, originalName = "java.lang.Process") +public class Process_Instrumentation { + @Trace + public int waitFor() { + Transaction transaction = AgentBridge.getAgent().getTransaction(false); + if (transaction != null) { + transaction.getTracedMethod().setMetricName("Java", "Process", "waitFor"); + } + return Weaver.callOriginal(); + } +} \ No newline at end of file diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/TransactionActivity.java b/newrelic-agent/src/main/java/com/newrelic/agent/TransactionActivity.java index 5346017509..bd44601e50 100644 --- a/newrelic-agent/src/main/java/com/newrelic/agent/TransactionActivity.java +++ b/newrelic-agent/src/main/java/com/newrelic/agent/TransactionActivity.java @@ -378,6 +378,31 @@ public void tracerFinished(Tracer tracer, int opcode) { /** * A serious internal error occurred. All data associated with this activity will be lost. * + * Note: We started seeing this error while investigating absurd metric values + * (e.g. - negative durations, or call counts in the hundreds of millions) + * The fix was to remove the ProcessPointCut and replace it with + * a weaver instrumentation module. + * We've left this code in place just in case we see the problem in other places. + * + * The offending code that produced this error looked something like this: + * It may be worth noting that myServiceMethod() was being called by another service method + * that was also annotated with @Trace(dispatcher = true) and that method was also + * called the same way. And that 3rd method was being called every 1 second + * by a ScheduledThreadPoolExecutor. And the mapOfStringToListOfStrings should have at least 2 entries. + * + * @Trace(dispatcher = true, metricName = "myMetricName") + * public void myServiceMethod () { + * ... + * CompletableFuture.allOf( + * mapOfStringToListOfStrings.entrySet().stream().map(entry -> CompletableFuture.runAsync(() -> { + * ... + * callSomeMethodThatUsesProcessWaitFor(); + * ... + * })).toArray(CompletableFuture[]::new) + * ).join(); + * ... + * } + * * @param tracer the tracer that completed, leading to the internal error detection. * @param opcode */ diff --git a/newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/pointcuts/ProcessPointCut.java b/newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/pointcuts/ProcessPointCut.java deleted file mode 100644 index fa906a2446..0000000000 --- a/newrelic-agent/src/main/java/com/newrelic/agent/instrumentation/pointcuts/ProcessPointCut.java +++ /dev/null @@ -1,36 +0,0 @@ -/* - * - * * Copyright 2020 New Relic Corporation. All rights reserved. - * * SPDX-License-Identifier: Apache-2.0 - * - */ - -package com.newrelic.agent.instrumentation.pointcuts; - -import com.newrelic.agent.Transaction; -import com.newrelic.agent.instrumentation.PointCutClassTransformer; -import com.newrelic.agent.instrumentation.TracerFactoryPointCut; -import com.newrelic.agent.instrumentation.classmatchers.ExactClassMatcher; -import com.newrelic.agent.tracers.ClassMethodSignature; -import com.newrelic.agent.tracers.DefaultTracer; -import com.newrelic.agent.tracers.Tracer; -import com.newrelic.agent.tracers.metricname.ClassMethodMetricNameFormat; - -@PointCut -public class ProcessPointCut extends TracerFactoryPointCut { - public static final String UNIXPROCESS_CLASS_NAME = "java/lang/UNIXProcess"; - public static final String PROCESS_IMPL_CLASS_NAME = "java/lang/ProcessImpl"; - - public ProcessPointCut(PointCutClassTransformer classTransformer) { - super(ProcessPointCut.class, ExactClassMatcher.or(PROCESS_IMPL_CLASS_NAME, UNIXPROCESS_CLASS_NAME), - createExactMethodMatcher("waitFor", "()I")); - classTransformer.getClassNameFilter().addIncludeClass("java/lang/ProcessImpl"); - classTransformer.getClassNameFilter().addIncludeClass("java/lang/UNIXProcess"); - } - - @Override - public Tracer doGetTracer(Transaction transaction, ClassMethodSignature sig, Object object, Object[] args) { - return new DefaultTracer(transaction, sig, object, new ClassMethodMetricNameFormat(sig, object)); - } - -} diff --git a/settings.gradle b/settings.gradle index 848313f152..eb7c1ccd5b 100644 --- a/settings.gradle +++ b/settings.gradle @@ -144,6 +144,7 @@ include 'instrumentation:jakarta.xml' include 'instrumentation:java.completable-future-jdk8' include 'instrumentation:java.completable-future-jdk8u40' include 'instrumentation:java.logging-jdk8' +include 'instrumentation:java.process' include 'instrumentation:java-io' include 'instrumentation:javax.xml' include 'instrumentation:jax-rs-1.0' From 22c4b0b3f2cc7c870c2a03fb0b0ea8c762900ddc Mon Sep 17 00:00:00 2001 From: John Bedell Date: Tue, 9 Jan 2024 11:56:38 -0500 Subject: [PATCH 2/3] Remove the associated test --- .../agent/instrumentation/ClassNameFilterFuncTest.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/functional_test/src/test/java/com/newrelic/agent/instrumentation/ClassNameFilterFuncTest.java b/functional_test/src/test/java/com/newrelic/agent/instrumentation/ClassNameFilterFuncTest.java index 0513933a73..3efdff4287 100644 --- a/functional_test/src/test/java/com/newrelic/agent/instrumentation/ClassNameFilterFuncTest.java +++ b/functional_test/src/test/java/com/newrelic/agent/instrumentation/ClassNameFilterFuncTest.java @@ -21,13 +21,6 @@ public void phase() { Assert.assertTrue(classTransformer.getClassNameFilter().isIncluded("com/sun/faces/lifecycle/Phase")); } - @Test - public void process() { - PointCutClassTransformer classTransformer = ServiceFactory.getClassTransformerService().getClassTransformer(); - Assert.assertTrue(classTransformer.getClassNameFilter().isIncluded("java/lang/UNIXProcess")); - Assert.assertTrue(classTransformer.getClassNameFilter().isIncluded("java/lang/ProcessImpl")); - } - @Test public void lifecycleImpl() { PointCutClassTransformer classTransformer = ServiceFactory.getClassTransformerService().getClassTransformer(); From 478e25e410e262ce4aa129ce089e3a3ef775bda9 Mon Sep 17 00:00:00 2001 From: John Bedell Date: Wed, 17 Jan 2024 09:39:42 -0500 Subject: [PATCH 3/3] Remove a stray System.out --- .../com/nr/agent/instrumentation/spring_webclient_60/Util.java | 1 - 1 file changed, 1 deletion(-) diff --git a/instrumentation/spring-webclient-6.0/src/main/java/com/nr/agent/instrumentation/spring_webclient_60/Util.java b/instrumentation/spring-webclient-6.0/src/main/java/com/nr/agent/instrumentation/spring_webclient_60/Util.java index 65ce9e8dd4..64705deae2 100644 --- a/instrumentation/spring-webclient-6.0/src/main/java/com/nr/agent/instrumentation/spring_webclient_60/Util.java +++ b/instrumentation/spring-webclient-6.0/src/main/java/com/nr/agent/instrumentation/spring_webclient_60/Util.java @@ -29,7 +29,6 @@ public class Util { public static Segment startSegment() { Transaction txn = AgentBridge.getAgent().getTransaction(false); - System.out.println(txn); return txn == null ? null : txn.startSegment("WebClient.exchange"); }