Reducing transitive dependency closure of System.Diagnostics.DiagnosticSource

[macrogreg]

This is a question / suggestion / request about the current RC of System.Diagnostics.DiagnosticSource.

Auto-instrumentation scenarios require an in-proc agent / tracer to load DiagnosticSource dynamically while performing non-trivial logic to avoid potential versioning conflicts with assemblies referenced by the application being instrumented. The logic would be considerably simpler and more robust if the DiagnosticSource-assembly itself did not have any additional dependencies.

On the recent NetFx and Net Core versions the System.Diagnostics.DiagnosticSource-NuGet depends on System.Memory.(When targeting older Net Core and Net Standard flavors there are more NuGet dependencies.) In turn, System.Memory requires (depending on the targeted runtime version): System.Runtime.CompilerServices.Unsafe, System.Buffers and System.Numerics.Vectors. Older runtime flavors require more.

I am guessing that the dependencies are on actor types rather than on data exchange types on public APIs. If so, it should be possible to build the types that are referenced from System.Memory and System.Runtime.CompilerServices.Unsafe directly into the DiagnosticSource-assembly (copy code or, better, just reference the respective sources at Framework build time). That way DiagnosticSource-assembly would not contain any downstream dependencies.

This would considerably simplify scenarios where an auto-instrumentation agent needs to dynamically load DiagnosticSource (and all its dependencies) without causing versioning conflicts with the instrumented application.

@noahfalk and @davmason have context on the related discussion in the OTel SIG.

This should be a build-logic change only. Can this be considered for Net 5?

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

[noahfalk]

Can this be considered for Net 5?

Sorry it is too late : ( At the runtime and BCL layers of the stack we are already winding down for the release. We can consider this for .NET 6. Also I believe some of those types are special-cased JIT intrinsics - copying the source to another assembly may cause significant performance regressions or outright failures because the JIT no longer recognizes the special casing applies.

[jkotas]

I am guessing that the dependencies are on actor types rather than on data exchange types on public APIs.

This does not appear to be the case:

    public readonly partial struct ActivitySpanId : System.IEquatable<System.Diagnostics.ActivitySpanId>
    {
        public static System.Diagnostics.ActivitySpanId CreateFromBytes(System.ReadOnlySpan<byte> idData);
        public static System.Diagnostics.ActivitySpanId CreateFromString(System.ReadOnlySpan<char> idData);
        public static System.Diagnostics.ActivitySpanId CreateFromUtf8String(System.ReadOnlySpan<byte> idData);
    }

System.ReadOnlySpan<T> lives in System.Memory.

[KalleOlaviNiemitalo]

#429 looks similar.

[macrogreg]

Yes, @KalleOlaviNiemitalo , it does, indeed appear to be the same issue. Looks like it's not super new. Since then we have more scenarios that highlight the problem (auto-instrumentation), wished we realized it in time. :)

[macrogreg]

@jkotas , thank you for spotting this, it's a bummer. I really wished we could instead use an API shape that takes an array and an index. It's not as modern as a ReadOnlySpan, but the dependencies are a real challenge (details below).

Btw, the latest RC takes a direct additional dependency on System.Runtime.CompilerServices.Unsafe (it existed before transitively via System.Memory). Is it one of those types that have special-cased JIT intrinsics (see @noahfalk 's comment above), or could a copy of it be theoretically built into the DiagnosticSource Nuget? (This is for my own education.)

The challenge presented by these dependencies to auto-instrumentation is not only that an auto-instrumentation agent becomes more complex. It overall limits the scenarios it can cover:

If DiagnosticSource did not have dependencies, we can use a logic such as (simplified):

• When the auto-instrumentation attaches, if DiagnosticSource is loaded, use the loaded version.
• Otherwise load a relatively recent one as a "fall-back" strategy.
• If an app was going to load DiagnisticSource later, it is still OK because of the stability of DiagnisticSource in respect to back-compatibility. It is generally OK for apps to use a newer version than they expected.

Consider now that an app did not have DiagnosticSource loaded, but it had an older version of System.Memory (or another dependency) loaded. If that version is older than required by our "fall-back" DiagnosticSource, we cannot load it. So applications that reference old versions of DiagnosticSource's dependencies without referencing a similarly old version of DiagnosticSource itself cannot be instrumented. (I'd love to find out that I am missing something, so please correct me if I am wrong. :) )

An even bigger issue (because it cannot be pre-preemptively detected) is that an app may want to load one of the DiagnosticSource dependencies after the tracer is attached. By then we have loaded the "fall-back", i.e. a recent DiagnosticSource version with respectively recent versions of its dependencies. Consider now that an app wanted to load an old version System.Memory (or another DiagnosticSource dependency). Instead, it will get a more recent version forced on it. If there is some backward incompatibility, there can be a very hard to debug issue or crash.

I will be researching workarounds and I would appreciate any thoughts you guys may have to share. Until then, I am somewhat concerned that these dependencies may put at risk the entire idea to have auto-instrumentation agents (incl. the OTel tracer) be Activity centric.

Thanks!

[jkotas]

System.Runtime.CompilerServices.Unsafe (it existed before transitively via System.Memory). Is it one of those types that have special-cased JIT intrinsics

System.Runtime.CompilerServices.Unsafe does not have special-cased JIT intrinsics.

[macrogreg]

@noahfalk - Could you please point to some information about

Also I believe some of those types are special-cased JIT intrinsics - copying the source to another assembly may cause significant performance regressions or outright failures because the JIT no longer recognizes the special casing applies.

As discussed in the OTel meeting, vendoring DiagnosticSource into a Tracer may be a fallback strategy when it is not on the default assembly loading path, and cannot be loaded dynamically from there. I'd like to understand what risks are associated with this.

Thank you!

[noahfalk]

I'm hoping we can stick to a simple rule of thumb, "don't transitively replicate any of DiagnosticSource.dll's dependencies." For any given dependency assembly we can either eliminate the requirement for it or accept that it will be needed for the scenario to work.

More info about intrinsics: https://github.com/dotnet/runtime/blob/master/docs/design/coreclr/botr/vectors-and-intrinsics.md