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

application crashed by native profiler after adding an instrumentation in integrations.json #1117

Closed
muhaook opened this issue Aug 20, 2022 · 11 comments

Comments

@muhaook
Copy link

muhaook commented Aug 20, 2022

Bug Report

Symptom

Describe the bug
My sample application is using odp.net (Oracle.ManagedDataAccess.dll) to access Oracle DB. As current Opentelemetry does not support Oracle.ManagedDataAccess.dll yet, I decided to generate traces/spans using my instrumentation:

  1. add a entry in integrations.json
  {
    "name": "Odp",
    "method_replacements": [
      {
        "caller": {},
        "target": {
          "assembly": "Oracle.ManagedDataAccess",
          "type": "Oracle.ManagedDataAccess.Client.OracleCommand",
          "method": "ExecuteNonQuery",
          "signature_types": [
            "System.Int32"
          ],
          "minimum_major": 1,
          "minimum_minor": 0,
          "minimum_patch": 0,
          "maximum_major": 65535,
          "maximum_minor": 65535,
          "maximum_patch": 65535
        },
        "wrapper": {
          "assembly": "OpenTelemetry.AutoInstrumentation",
          "type": "OpenTelemetry.AutoInstrumentation.Instrumentations.Odp.OdpClientIntegration",
          "action": "CallTargetModification"
        }
      }
    ]
  }
  1. create class OpenTelemetry.AutoInstrumentation.Instrumentations.Odp.OdpClientIntegration which is nearly empty at this stage.

my sample application crashed after doing the above. error message:

Unhandled Exception: System.IO.FileLoadException: Could not load file or assembly 'OpenTelemetry.AutoInstrumentation, Version=0.2.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. A strongly-named assembly is required. (Exception from HRESULT: 0x80131044)
   at Oracle.ManagedDataAccess.Client.OracleCommand.ExecuteNonQuery()
   at ConsoleApp.Program.Update() in C:\ConsoleApp\Program.cs:line 160
   at ConsoleApp.Program.Main(String[] args) in C:\ConsoleApp\Program.cs:line 20

Expected behavior
at this stage, I did not expect to see any traces/spans yet, but my sample app should not crash.

Runtime environment (please complete the following information):

  • OpenTelemetry Automatic Instrumentation version: 0.2.0
  • OS: Windows 10
  • .NET version: NET Framework 4.6.2

attached please find logs
logs.zip

@pjanotti
Copy link
Contributor

Hi @muhaook - if I understand correctly you are trying to create an automatic instrumentation, so something like what was done at https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/tree/main/src/OpenTelemetry.AutoInstrumentation/Instrumentations

Could you confirm if that is really the case?

@RassK
Copy link
Contributor

RassK commented Aug 23, 2022

"assembly": "OpenTelemetry.AutoInstrumentation", must point to the wrapper assembly (basically assembly where's your myinstrumentation.cs is defined). In that case I guess it's ConsoleApp.

The approach is currently not documented, but integrations.json is needed to inject into bytecode your hooks to help generate spans. The preferred way would be source based instrumentation, but that expects already some kind of diagnostics layer you can reuse to create spans.

I'm really wondering now, if bytecode based instrumentation could be done using our plugin system 🤔

@muhaook
Copy link
Author

muhaook commented Aug 23, 2022

Thanks @pjanotti @RassK , yes I am trying to create an automatic instrumentation.

I decompiled the ODP.NET library and had a quick look, there isn't any sources (ActivitySource/DiagnosticSource/etc) being used there. So looks like the automatic (bytecode based) instrumentation is the only way to generate telemetry in ODP.NET lib.

I did inject entries in integrations.json and created functions such as OnMethodBegin and OnMethodEnd. I am using console apps to test the automatic instrumentation for .net framework, .net core 3.1, and .net 6. both .net core and .net 6 work as expected. the only issue is .net framework (4.6.2) . Attached please find the console app I used to test .net framework 4.6.2. looks like something was wrong right after IL bytecode was added around OracleCommand.ExecuteNonQuery().
ConsoleApp462.zip

@RassK
Copy link
Contributor

RassK commented Aug 24, 2022

I'm just currently guessing: Oracle.ManagedDataAccess is strong named library, IL injection seems to work but it causes non-strong named library load (in this case OpenTelemetry.AutoInstrumentation).

@zacharycmontoya you probably have better insights to that?

@RassK
Copy link
Contributor

RassK commented Aug 24, 2022

I can definitely now confirm it. After signing OpenTelemetry.AutoInstrumentation the error is gone and injected Console.WriteLine is displayed on ExecuteNonQuery call.

@muhaook
Copy link
Author

muhaook commented Aug 24, 2022

Hi @RassK , I can see Oracle.ManagedDataAccess.dll is strong-named in .NET framework 462 and .NET Core 3.1 / .NET 6 .

ConsoleAppNet6\bin\Debug\net6.0> sn -v .\Oracle.ManagedDataAccess.dll

Microsoft (R) .NET Framework Strong Name Utility  Version 4.0.30319.0
Copyright (c) Microsoft Corporation.  All rights reserved.

Assembly '.\Oracle.ManagedDataAccess.dll' is valid

But the issue happened in .NET framework 462 only. Does it mean strong-named assembly has different behavior in.NET framework 462vs .NET core 3.1 / .NET 6 ?

@RassK
Copy link
Contributor

RassK commented Aug 24, 2022

They (Microsoft) reworked the strong naming concept in .NET Core, so I guess all the restrictions are removed there.

@muhaook
Copy link
Author

muhaook commented Aug 24, 2022

saw this in this doc just now

For .NET Core and .NET 5+, strong-named assemblies do not provide material benefits. The runtime never validates the strong-name signature, nor does it use the strong-name for assembly binding.

@Kielek
Copy link
Contributor

Kielek commented Aug 24, 2022

@muhaook, we have a SIG meeting few minutes ago.

Two tasks are created to cover your issues

Thanks for your feedback. It will be great if you can share further information/any missing instrumentation/scenarios you need. It will be great if you can share any improvements in our code base also.

@pjanotti
Copy link
Contributor

pjanotti commented Sep 2, 2022

@muhaook I just created a PR to fix the strong name issue, see #1153

@muhaook
Copy link
Author

muhaook commented Sep 22, 2022

@pjanotti , it works, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants