Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swap the ProcessPointCut over to a weaver instrumentation module to … #1685

Merged
merged 3 commits into from
Jan 29, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
20 changes: 20 additions & 0 deletions instrumentation/java.process/build.gradle
Original file line number Diff line number Diff line change
@@ -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,)'
}
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down

This file was deleted.

1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down