diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationContext.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationContext.java index 5a7dda111d2..5768cff4d3f 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationContext.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationContext.java @@ -24,7 +24,10 @@ private InstrumentationContext() {} public static ContextStore get( final Class keyClass, final Class contextClass) { throw new RuntimeException( - "Calls to this method will be rewritten by Instrumentation Context Provider (e.g. FieldBackedProvider)"); + "Calls to this method will be rewritten by Instrumentation Context Provider (e.g. FieldBackedProvider)." + + " If you get this exception, this method has not been rewritten." + + " Ensure instrumentation class has a contextStore method and the call to InstrumentationContext.get happens directly in an instrumentation Advice class." + + " See how_instrumentations_work.md for details."); } /** @@ -35,6 +38,9 @@ public static ContextStore get( */ public static ContextStore get(final String keyClass, final String contextClass) { throw new RuntimeException( - "Calls to this method will be rewritten by Instrumentation Context Provider (e.g. FieldBackedProvider)"); + "Calls to this method will be rewritten by Instrumentation Context Provider (e.g. FieldBackedProvider)." + + " If you get this exception, this method has not been rewritten." + + " Ensure instrumentation class has a contextStore method and the call to InstrumentationContext.get happens directly in an instrumentation Advice class." + + " See how_instrumentations_work.md for details."); } } diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationErrors.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationErrors.java index 580b1557f3c..c18534a73e9 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationErrors.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InstrumentationErrors.java @@ -1,21 +1,61 @@ package datadog.trace.bootstrap; +import java.io.PrintWriter; +import java.io.StringWriter; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicLong; public class InstrumentationErrors { private static final AtomicLong COUNTER = new AtomicLong(); - public static long getErrorCount() { - return COUNTER.get(); + private static final class Detailed { + static final List ERRORS = new CopyOnWriteArrayList<>(); } - @SuppressWarnings("unused") - public static void incrementErrorCount() { + private static volatile boolean detailed; + + /** Record an error occurred without any detail about it. */ + public static void recordError() { + COUNTER.incrementAndGet(); + } + + /** Record an error occurred, including its stack trace. */ + public static Throwable recordError(Throwable error) { COUNTER.incrementAndGet(); + StringWriter detail = new StringWriter(); + error.printStackTrace(new PrintWriter(detail)); + Detailed.ERRORS.add(detail.toString()); + detailed = true; + return error; // keep throwable at top of the stack } // Visible for testing - public static void resetErrorCount() { + public static void resetErrors() { COUNTER.set(0); + if (detailed) { + Detailed.ERRORS.clear(); + detailed = false; + } + } + + /** + * @return {@code true} if no errors were recorded; otherwise {@code false} + */ + public static boolean noErrors() { + return COUNTER.get() == 0; + } + + /** + * @return a human-readable description of the errors recorded so far + */ + public static String describeErrors() { + StringBuilder buf = new StringBuilder().append(COUNTER.get()).append(" instrumentation errors"); + if (detailed) { + for (String error : Detailed.ERRORS) { + buf.append("\n---\n").append(error); + } + } + return buf.toString(); } } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java index 20099d76a37..2fd96e2edf1 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/ExceptionHandlers.java @@ -27,6 +27,8 @@ public class ExceptionHandlers { private final Size size = new StackManipulation.Size(-1, 3); private final boolean appSecEnabled = InstrumenterConfig.get().getAppSecActivation() != ProductActivation.FULLY_DISABLED; + private final boolean detailedErrors = + InstrumenterConfig.get().isDetailedInstrumentationErrors(); @Override public boolean isValid() { @@ -43,7 +45,7 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) // // BlockingExceptionHandler.rethrowIfBlockingException(t); // try { - // InstrumentationErrors.incrementErrorCount(); + // InstrumentationErrors.recordError(); // org.slf4j.LoggerFactory.getLogger((Class)ExceptionLogger.class) // .debug("Failed to handle exception in instrumentation for ...", t); // } catch (Throwable t2) { @@ -53,7 +55,7 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) // // BlockingExceptionHandler.rethrowIfBlockingException(t); // try { - // InstrumentationErrors.incrementErrorCount(); + // InstrumentationErrors.recordError(); // org.slf4j.LoggerFactory.getLogger((Class)ExceptionLogger.class) // .error("Failed to handle exception in instrumentation for ...", t); // System.exit(1); @@ -76,17 +78,28 @@ public Size apply(final MethodVisitor mv, final Implementation.Context context) Opcodes.INVOKESTATIC, "datadog/trace/bootstrap/blocking/BlockingExceptionHandler", "rethrowIfBlockingException", - "(Ljava/lang/Throwable;)Ljava/lang/Throwable;"); + "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", + false); } mv.visitTryCatchBlock(logStart, logEnd, eatException, "java/lang/Throwable"); mv.visitLabel(logStart); - // invoke incrementAndGet on our exception counter - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/bootstrap/InstrumentationErrors", - "incrementErrorCount", - "()V"); + // record instrumentation error + if (detailedErrors) { + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/bootstrap/InstrumentationErrors", + "recordError", + "(Ljava/lang/Throwable;)Ljava/lang/Throwable;", + false); + } else { + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/bootstrap/InstrumentationErrors", + "recordError", + "()V", + false); + } // stack: (top) throwable mv.visitLdcInsn(Type.getType("L" + HANDLER_NAME + ";")); mv.visitMethodInsn( diff --git a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy index 0c9bfd035b8..be0b321216e 100644 --- a/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy +++ b/dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy @@ -47,6 +47,7 @@ import datadog.trace.api.Pair import datadog.trace.api.ProcessTags import datadog.trace.api.TraceConfig import datadog.trace.api.config.GeneralConfig +import datadog.trace.api.config.TraceInstrumentationConfig import datadog.trace.api.config.TracerConfig import datadog.trace.api.datastreams.AgentDataStreamsMonitoring import datadog.trace.api.datastreams.DataStreamsTransactionExtractor @@ -79,7 +80,6 @@ import java.nio.ByteBuffer import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit import java.util.concurrent.TimeoutException -import java.util.concurrent.atomic.AtomicInteger import net.bytebuddy.agent.ByteBuddyAgent import net.bytebuddy.agent.builder.AgentBuilder import net.bytebuddy.description.type.TypeDescription @@ -169,10 +169,6 @@ abstract class InstrumentationSpecification extends DDSpecification implements A @Shared Set TRANSFORMED_CLASSES_TYPES = Sets.newConcurrentHashSet() - @SuppressWarnings('PropertyName') - @Shared - AtomicInteger INSTRUMENTATION_ERROR_COUNT = new AtomicInteger(0) - // don't use mocks because it will break too many exhaustive interaction-verifying tests @SuppressWarnings('PropertyName') @Shared @@ -353,6 +349,8 @@ abstract class InstrumentationSpecification extends DDSpecification implements A @SuppressForbidden void setupSpec() { + InstrumentationErrors.resetErrors() + AgentMeter.registerIfAbsent( STATS_D_CLIENT, new MonitoringImpl(STATS_D_CLIENT, 10, TimeUnit.SECONDS), @@ -423,6 +421,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A .hasNext(): "No instrumentation found" activeTransformer = AgentInstaller.installBytebuddyAgent( INSTRUMENTATION, true, AgentInstaller.getEnabledSystems(), this) + + // check for instrumentation issues during installation + assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors() } protected String idGenerationStrategyName() { @@ -431,12 +432,15 @@ abstract class InstrumentationSpecification extends DDSpecification implements A /** Override to set config before the agent is installed */ protected void configurePreAgent() { + injectSysConfig(TraceInstrumentationConfig.DETAILED_INSTRUMENTATION_ERRORS, "true") injectSysConfig(TracerConfig.SCOPE_ITERATION_KEEP_ALIVE, "1") // don't let iteration spans linger injectSysConfig(GeneralConfig.DATA_STREAMS_ENABLED, String.valueOf(isDataStreamsEnabled())) injectSysConfig(GeneralConfig.DATA_JOBS_ENABLED, String.valueOf(isDataJobsEnabled())) } void setup() { + InstrumentationErrors.resetErrors() // reset for each test + configureLoggingLevels() assertThreadsEachCleanup = false @@ -472,7 +476,6 @@ abstract class InstrumentationSpecification extends DDSpecification implements A if (forceAppSecActive) { ActiveSubsystems.APPSEC_ACTIVE = true } - InstrumentationErrors.resetErrorCount() ProcessTags.reset() } @@ -514,7 +517,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A spanFinishLocations.clear() originalToTrackingSpan.clear() } - assert InstrumentationErrors.errorCount == 0 + + // check for instrumentation issues while running each test + assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors() } private void doCheckRepeatedFinish() { @@ -556,8 +561,6 @@ abstract class InstrumentationSpecification extends DDSpecification implements A cleanupAfterAgent() // All cleanup should happen before these assertion. If not, a failing assertion may prevent cleanup - assert INSTRUMENTATION_ERROR_COUNT.get() == 0: INSTRUMENTATION_ERROR_COUNT.get() + " Instrumentation errors during test" - assert TRANSFORMED_CLASSES_TYPES.findAll { GlobalIgnores.isAdditionallyIgnored(it.getActualName()) }.isEmpty(): "Transformed classes match global libraries ignore matcher" @@ -658,9 +661,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A return } + InstrumentationErrors.recordError(throwable) println "Unexpected instrumentation error when instrumenting ${typeName} on ${classLoader}" throwable.printStackTrace() - INSTRUMENTATION_ERROR_COUNT.incrementAndGet() } @Override diff --git a/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java b/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java index 58f1831c0f3..a82c8319abd 100644 --- a/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java +++ b/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java @@ -13,6 +13,7 @@ import datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers; import datadog.trace.api.Config; import datadog.trace.api.IdGenerationStrategy; +import datadog.trace.bootstrap.InstrumentationErrors; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer.TracerAPI; import datadog.trace.common.writer.ListWriter; @@ -20,6 +21,7 @@ import datadog.trace.core.DDSpan; import datadog.trace.core.PendingTrace; import datadog.trace.core.TraceCollector; +import datadog.trace.junit.utils.config.WithConfig; import datadog.trace.junit.utils.context.AllowContextTestingExtension; import java.lang.instrument.ClassFileTransformer; import java.lang.instrument.Instrumentation; @@ -51,6 +53,7 @@ *
  • {@code @AfterAll}: Closes the tracer and removes the agent transformer * */ +@WithConfig(key = "detailed.instrumentation.errors", value = "true") @ExtendWith({TestClassShadowingExtension.class, AllowContextTestingExtension.class}) public abstract class AbstractInstrumentationTest { static final Instrumentation INSTRUMENTATION = ByteBuddyAgent.getInstrumentation(); @@ -66,6 +69,8 @@ public abstract class AbstractInstrumentationTest { @BeforeAll static void initAll() { + InstrumentationErrors.resetErrors(); + // If this fails, it's likely the result of another test loading Config before it can be // injected into the bootstrap classpath. assertNull(Config.class.getClassLoader(), "Config must load on the bootstrap classpath."); @@ -97,10 +102,14 @@ static void initAll() { activeTransformer = AgentInstaller.installBytebuddyAgent( INSTRUMENTATION, true, AgentInstaller.getEnabledSystems(), transformerListener); + + // check for instrumentation issues during installation + assertTrue(InstrumentationErrors.noErrors(), InstrumentationErrors::describeErrors); } @BeforeEach public void init() { + InstrumentationErrors.resetErrors(); // reset for each test tracer.flush(); writer.start(); } @@ -108,6 +117,9 @@ public void init() { @AfterEach public void tearDown() { tracer.flush(); + + // check for instrumentation issues while running each test + assertTrue(InstrumentationErrors.noErrors(), InstrumentationErrors::describeErrors); } @AfterAll diff --git a/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java b/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java index 16ba4954efc..a865bf374d1 100644 --- a/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java +++ b/dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java @@ -1,13 +1,12 @@ package datadog.trace.agent.test; -import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.Sets; import datadog.trace.agent.tooling.bytebuddy.matcher.GlobalIgnores; +import datadog.trace.bootstrap.InstrumentationErrors; import de.thetaphi.forbiddenapis.SuppressForbidden; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; import net.bytebuddy.agent.builder.AgentBuilder; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.dynamic.DynamicType; @@ -18,7 +17,6 @@ public class ClassFileTransformerListener implements AgentBuilder.Listener { final Set transformedClassesNames = Sets.newConcurrentHashSet(); final Set transformedClassesTypes = Sets.newConcurrentHashSet(); - final AtomicInteger instrumentationErrorCount = new AtomicInteger(0); @Override public void onTransformation( @@ -45,10 +43,10 @@ public void onError( return; } + InstrumentationErrors.recordError(throwable); System.out.println( "Unexpected instrumentation error when instrumenting " + typeName + " on " + classLoader); throwable.printStackTrace(); - instrumentationErrorCount.incrementAndGet(); } @Override @@ -70,9 +68,6 @@ public void onComplete( } public void verify() { - // Check instrumentation errors - int errorCount = this.instrumentationErrorCount.get(); - assertEquals(0, errorCount, errorCount + " instrumentation errors during test"); // Check effectively transformed classes that should have been ignored assertTrue( this.transformedClassesTypes.stream() diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java index 0d21a7e371d..6d44cf8b249 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/TraceInstrumentationConfig.java @@ -15,6 +15,7 @@ public final class TraceInstrumentationConfig { public static final String CODE_ORIGIN_MAX_USER_FRAMES = "code.origin.max.user.frames"; public static final String TRACE_ENABLED = "trace.enabled"; public static final String INTEGRATIONS_ENABLED = "integrations.enabled"; + public static final String DETAILED_INSTRUMENTATION_ERRORS = "detailed.instrumentation.errors"; public static final String TRACE_EXTENSIONS_PATH = "trace.extensions.path"; diff --git a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java index e8627a5a852..74bff640024 100644 --- a/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java +++ b/internal-api/src/main/java/datadog/trace/api/InstrumenterConfig.java @@ -56,6 +56,7 @@ import static datadog.trace.api.config.TraceInstrumentationConfig.AXIS_TRANSPORT_CLASS_NAME; import static datadog.trace.api.config.TraceInstrumentationConfig.CODE_ORIGIN_FOR_SPANS_ENABLED; import static datadog.trace.api.config.TraceInstrumentationConfig.CODE_ORIGIN_FOR_SPANS_INTERFACE_SUPPORT; +import static datadog.trace.api.config.TraceInstrumentationConfig.DETAILED_INSTRUMENTATION_ERRORS; import static datadog.trace.api.config.TraceInstrumentationConfig.EXPERIMENTAL_DEFER_INTEGRATIONS_UNTIL; import static datadog.trace.api.config.TraceInstrumentationConfig.HTTP_URL_CONNECTION_CLASS_NAME; import static datadog.trace.api.config.TraceInstrumentationConfig.INSTRUMENTATION_CONFIG_ID; @@ -145,6 +146,7 @@ public class InstrumenterConfig { private final boolean triageEnabled; private final boolean integrationsEnabled; + private final boolean detailedInstrumentationErrors; private final boolean codeOriginEnabled; private final boolean codeOriginInterfaceSupport; @@ -249,6 +251,8 @@ private InstrumenterConfig() { integrationsEnabled = configProvider.getBoolean(INTEGRATIONS_ENABLED, DEFAULT_INTEGRATIONS_ENABLED); + detailedInstrumentationErrors = + configProvider.getBoolean(DETAILED_INSTRUMENTATION_ERRORS, false); codeOriginEnabled = configProvider.getBoolean( @@ -408,6 +412,10 @@ public boolean isIntegrationsEnabled() { return integrationsEnabled; } + public boolean isDetailedInstrumentationErrors() { + return detailedInstrumentationErrors; + } + /** * isIntegrationEnabled determines whether an integration under the specified name(s) is enabled * according to the following list of configurations, from highest to lowest precedence: diff --git a/metadata/supported-configurations.json b/metadata/supported-configurations.json index 01087b11aa4..32e40412662 100644 --- a/metadata/supported-configurations.json +++ b/metadata/supported-configurations.json @@ -11792,6 +11792,14 @@ "default": "true", "aliases": ["DD_TRACE_INTEGRATION_JAVA_MODULE_ENABLED", "DD_INTEGRATION_JAVA_MODULE_ENABLED"] } + ], + "DD_DETAILED_INSTRUMENTATION_ERRORS": [ + { + "version": "A", + "type": "boolean", + "default": null, + "aliases": [] + } ] }, "deprecations": {}