opentelemetry-dotnet-instrumentation icon indicating copy to clipboard operation
opentelemetry-dotnet-instrumentation copied to clipboard

[Research] How to enable library dependent instrumentations on .NET Framework

Open Kielek opened this issue 3 years ago • 5 comments
trafficstars

Research

Are you requesting automatic instrumentation for a framework or library? Please describe.

For now, we have two library-dependent instrumentation in our code base:

  • MongoDb,
  • MySql.Data.

We expect more to implement in a similar way. Both are working on .NET Core 3.1+.

I was able to execute MySql.Data instrumentation also on .NET Framework if the instrumented application was compiled with exactly the same version as the OpenTelemetry.Instrumentation.MySqlData.

Potential solutions

  • Create a custom assembly resolver to resolve the dependency from the instrumented library. It could be tricky, especially if there is a non-standard resolver already used.
  • Assembly redirects config file.
  • Other ideas(?)

Kielek avatar Jul 21 '22 06:07 Kielek

A custom assembly resolver could conflict with a user-defined assembly resolver. It would have to be possible to disable this functionality. When it would be disabled (because the user has e a problem with it) then the customer could configure assembly redirects to resolve the assembly version conflict.

pellared avatar Jul 21 '22 06:07 pellared

Assembly redirects seems to be the way to go in this case. It is not user friendly but it is the mechanism provided by the .NET Framework.

pjanotti avatar Jul 28 '22 18:07 pjanotti

Some of the work with this issue involves investigating if there are other options, in addition to the binding redirects. For now we're simplifying things so that .net core and .net 6 users can try out the expanded support sooner.

On a separate note, I'm interested in understanding how many users of .net framework are interested in trying out this project.

nrcventura avatar Jul 28 '22 19:07 nrcventura

Oh I have somehow skipped this issue.

I was thinking it could be done very similarly to MongoDB instrumentation. Type.GetType will trigger assembly load if the middleman assembly is missing, otherwise it returns the user loaded type. So all missed assembly loads will be redirected to the loader and loader can pick it up from the main dir. .NET Core is using additional probing path and loads the newest version from the store, but since .NET Fx doesn't have additional probing paths configurable like that, the dll must exist in the OTel Autoinstrumentation dir or in the application dir.

If we add new library project (similar like OpenTelemetry.AutoInstrumentation.AdditionalDeps) but for .NET Fx only and reference all of the middleman nuget packages there, then during the build they will be placed exactly to the right dir.

Conflict resolution is still the same:

  • .NET Core needs adjusting nuget package
  • .NET Fx needs binding redirects

NB: Conflict in this case means user is referencing the middleman assembly but we don't like the referenced version.

RassK avatar Aug 24 '22 16:08 RassK

Comment why I don't like Nuget package as a solution for this specific issue:

  • It's going to litter customer's application output with unnecessary dlls.
  • NET Core has already different pattern, using AdditionalDeps / Shared store.
  • Shipping these dlls in net462 dir is very similar to shared store solution.
  • Matching sub-dependencies could cause unwanted upgrades to customer dependencies, even when the original dll will be never loaded.
  • We will use customer's version if application references one or dyn load latest version ourself.
  • Because these middle drivers reference main instrumented library, Nuget can force unwanted upgrade to the main library.

RassK avatar Sep 01 '22 14:09 RassK

The most ambitious and flexible solution would be to re-write the IL tokens at execution time and completely hide the issue from the user. The re-writing would be on the bytecode IL: it would be compiled against specific versions but would be re-written as the versions that are available at runtime. This needs new code for the IL rewrite used by bytecode instrumentation but it is the way to do it transparently. It can also make easier to write the bytecode instrumentations: they can be written in a fashion that actually uses the actual target types and methods making it easier to create and more accessible to the developer community in general.

It is unclear to me the potential performance impact of this, that said it should happen only once per ReJIT of a bytecode instrumentation target.

At this point I'm unsure if it is worth the trouble: are there enough bytecode instrumentations to justify the effort?

pjanotti avatar Oct 05 '22 03:10 pjanotti

ILMerge, see #1418, may also be considered.

pjanotti avatar Oct 13 '22 18:10 pjanotti

It has to be noted that there are 2 different problems here: MongoDb: Requires translation package from internal diagnostics to OTel. MySql.Data: Requires instrumentation package.

IL merge won't solve the first issue perfectly as it's going to limit supported version range even more. For MongoDB at least, dynamic loading is much better. Dynamic loading works even better with .NET shared store, as you can include multiple versions.

RassK avatar Oct 17 '22 12:10 RassK

A combination of #1825 plus #1824 should suffice here. So after #1824 is merged we can revisit this item.

cc @RassK

pjanotti avatar Jan 23 '23 18:01 pjanotti

Lets check if MongoDB can work after @RassK changes. If not, lets postpone after 1.0.0.

It is unlikely that MySql.Data will work for all cases with current support (changes in public API even between 8.0.30 and 8.0.31).

Kielek avatar Jan 23 '23 19:01 Kielek

Consider closing this issue. MongoDB is working fine both on .NET Framework and .NET. Other libraries probably can be instrumented in the similar way - it should be handled case by case.

Kielek avatar Apr 17 '23 05:04 Kielek

We have automatic redirection and a kind of "roll forward" for .NET Framework, plus, the model to instrument libraries on .NET Framework. Closing per discussion above.

pjanotti avatar Apr 19 '23 17:04 pjanotti