Log error if missing bytecode instrumentation method (#1520)

CHANGELOG.md

@@ -23,6 +23,11 @@ This component adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.h
 - Get rid of unnecessary service restarts during the IIS unregistration,
   in the PowerShell script module.
 
+### Added
+
+- Error message on the native log if bytecode instrumentation type is missing all
+  instrumentation methods [#1499](https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/1499).
+
 ## [0.4.0-beta.1](https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/releases/tag/v0.4.0-beta.1)
 
 ### Added

src/OpenTelemetry.AutoInstrumentation.Native/cor_profiler.cpp

@@ -2494,14 +2494,47 @@ HRESULT CorProfiler::CallTarget_RewriterCallback(RejitHandlerModule* moduleHandl
             &wrapper_type_def);
         if (FAILED(hr) || wrapper_type_def == mdTypeDefNil)
         {
-            Logger::Warn("*** CallTarget_RewriterCallback() Failed for: ", caller->type.name, ".", caller->name,
+            Logger::Error("*** CallTarget_RewriterCallback() Failed for: ", caller->type.name, ".", caller->name,
                         "() integration type not found on the managed profiler module HRESULT=", HResultStr(hr),
                         " IntegrationType=", wrapper.type_name);
             return S_FALSE;
         }
 
-        // TODO: Use the method signature to validate also the method, tracking via
-        // https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issue/1499 
+        // CallTarget instrumentation doesn't inject calls to the instrumentation methods via IL rewrite.
+        // It injects the OpenTelemetry.AutoInstrumentation.CallTarget.CallTargetInvoker, written in managed code,
+        // that uses reflection to find the expected instrumentation methods on the instrumentation wrapper type.
+        // If the wrapper type doesn't have any of the expected instrumentation methods "nothing happens", but,
+        // the JIT code of the targeted method is modified anyway. To avoid injecting instrumentation that does
+        // nothing and give a clear error message the code below ensures that at least one of the expected methods is
+        // implemented on the wrapper type.
+        static const LPCWSTR expected_wrapper_methods[] = { WStr("OnMethodBegin"), WStr("OnMethodEnd"), WStr("OnAsyncMethodEnd") };
+        bool found_wrapper_method = false;
+        for (LPCWSTR expected_wrapper_method : expected_wrapper_methods)
+        {
+            mdMethodDef wrapper_method_def[1];
+            HCORENUM phEnum = NULL;
+            ULONG cTokens,
+            hr = instrumentation_module_metadata->metadata_import->EnumMethodsWithName(
+                &phEnum,
+                wrapper_type_def,
+                expected_wrapper_method,
+                wrapper_method_def,
+                1,
+                &cTokens);
+            if (hr == S_OK && cTokens > 0)
+            {
+                found_wrapper_method = true;
+                break;
+            }
+        }
+
+        if (!found_wrapper_method)
+        {
+            Logger::Error("*** CallTarget_RewriterCallback() Failed for: ", caller->type.name, ".", caller->name,
+                "() integration type found but none of the wrapper methods expected by CallTargetInvoker was found ",
+                "IntegrationType=", wrapper.type_name);
+            return S_FALSE;
+        }
     }
 
     ModuleID module_id = moduleHandler->GetModuleId();

src/OpenTelemetry.AutoInstrumentation.Native/metadata_builder.cpp

@@ -129,7 +129,7 @@ HRESULT MetadataBuilder::StoreWrapperMethodRef(const MethodReplacement& method_r
 
     // If the signature data size is greater than zero means we need to load the methodRef
     // for CallSite instrumentation.
-    // In case of the signature data size is zero we asume we are in a calltarget scenario
+    // In case of the signature data size is zero we assume we are in a CallTarget scenario
     // where we use the TypeRef but not a MemberRef.
 
     if (signature_data.size() > 0)

test/IntegrationTests/StrongNamedTests.cs

@@ -42,6 +42,10 @@ public class StrongNamedTests : TestHelper
         SetEnvironmentVariable("OTEL_DOTNET_AUTO_INTEGRATIONS_FILE", integrationsFile);
         RunTestApplication(otlpTraceCollectorPort: collector.Port);
 
+        // TODO: When native logs are moved to an EventSource implementation check for the log
+        // TODO: entries reporting the missing instrumentation type and missing instrumentation methods.
+        // TODO: See https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/960
+
         collector.AssertExpectations();
     }
 }

test/IntegrationTests/StrongNamedTestsIntegrations.json

@@ -46,6 +46,28 @@
           "type": "OpenTelemetry.AutoInstrumentation.Instrumentations.Validations.MissingInstrumentationType",
           "action": "CallTargetModification"
         }
+      },
+      {
+        "caller": {},
+        "target": {
+          "assembly": "TestLibrary.InstrumentationTarget",
+          "type": "TestLibrary.InstrumentationTarget.Command",
+          "method": "InstrumentationTargetMissingBytecodeInstrumentationMethod",
+          "signature_types": [
+            "System.Void"
+          ],
+          "minimum_major": 1,
+          "minimum_minor": 0,
+          "minimum_patch": 0,
+          "maximum_major": 1,
+          "maximum_minor": 65535,
+          "maximum_patch": 65535
+        },
+        "wrapper": {
+          "assembly": "OpenTelemetry.AutoInstrumentation",
+          "type": "OpenTelemetry.AutoInstrumentation.DuckTyping.DuckAttribute",
+          "action": "CallTargetModification"
+        }
       }
     ]
   }

test/test-applications/integrations/TestApplication.StrongNamed/Program.cs

@@ -25,5 +25,6 @@ public static class Program
         var command = new Command();
         command.Execute();
         command.InstrumentationTargetMissingBytecodeInstrumentationType();
+        command.InstrumentationTargetMissingBytecodeInstrumentationMethod();
     }
 }

test/test-applications/integrations/dependency-libs/TestLibrary.InstrumentationTarget/Command.cs

@@ -29,4 +29,9 @@ public class Command
     {
         Thread.Sleep(0); // Just to have some call to outside code.
     }
+
+    public void InstrumentationTargetMissingBytecodeInstrumentationMethod()
+    {
+        Thread.Yield(); // Just to have some call to outside code.
+    }
 }

tools/IntegrationsJsonGenerator/Program.cs

@@ -65,7 +65,7 @@ foreach (var typeInfo in autoInstrumentationLib.GetTypes())
 var productionIntegrations = integrations.Where(x => x.Key != "StrongNamedValidation").Select(x => x.Value)
     .OrderBy(x => x.Name).ToArray();
 
-var testIntegrations = integrations.Where(x => x.Key == "StrongNamedValidation").Select(x => x.Value)
+var testIntegrations = integrations.Where(x => x.Key == "StrongNamedValidation").Select(x => AppendMockIntegrations(x.Value))
     .OrderBy(x => x.Name).ToArray();
 
 UpdateIntegrationFile(Path.Combine(solutionFolder, "integrations.json"), productionIntegrations);
@@ -164,3 +164,48 @@ void UpdateIntegrationFile(string filePath, Integration[] productionIntegrations
 
 static string GetSourceFilePathName([CallerFilePath] string callerFilePath = null)
     => callerFilePath ?? string.Empty;
+
+static Integration AppendMockIntegrations(Integration testIntegration)
+{
+    // Add some special cases used by the integration tests. This way the integrations
+    // file used in the integrations test doesn't change on each run of the tool.
+    var targetAssembly = testIntegration.MethodReplacements[0].Target.Assembly;
+    var targetType = testIntegration.MethodReplacements[0].Target.Type;
+    var targetSignatureTypes = testIntegration.MethodReplacements[0].Target.SignatureTypes;
+
+    testIntegration.MethodReplacements.Add(new MethodReplacement
+    {
+        Target = new Target
+        {
+            Assembly = testIntegration.MethodReplacements[0].Target.Assembly,
+            Type = targetType,
+            Method = "InstrumentationTargetMissingBytecodeInstrumentationType",
+            SignatureTypes = targetSignatureTypes,
+            MaximumMajor = 1,
+            MinimumMajor = 1,
+        },
+        Wrapper = new Wrapper
+        {
+            Type = "OpenTelemetry.AutoInstrumentation.Instrumentations.Validations.MissingInstrumentationType",
+        },
+    });
+
+    testIntegration.MethodReplacements.Add(new MethodReplacement
+    {
+        Target = new Target
+        {
+            Assembly = testIntegration.MethodReplacements[0].Target.Assembly,
+            Type = targetType,
+            Method = "InstrumentationTargetMissingBytecodeInstrumentationMethod",
+            SignatureTypes = targetSignatureTypes,
+            MaximumMajor = 1,
+            MinimumMajor = 1,
+        },
+        Wrapper = new Wrapper
+        {
+            Type = "OpenTelemetry.AutoInstrumentation.DuckTyping.DuckAttribute",
+        },
+    });
+
+    return testIntegration;
+}