DiagnosticSource instrumentation helpers as public?

[cijothomas]

This folder (https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/Instrumentation) contains the classes which helps subscribe to DiagnosticSource based events. Its to be used by those instrumentations, which are relying on the instrumented library to publish events using DiagnosticSource. These are currently exposed as public APIs from the Otel SDK nuget.

Opening this issue, to discuss if we need to make this a public api.

Proposal:
Don't expose this as public API. The instrumentations within this repo and used a shared file, to avoid copy-paste.
For any new instrumentations, the recommended approach is to use ActivitySource and those libraries will not require DS subscription mechanisms anyway.

[cijothomas]

Plan of action for GA Release:

1. Rename AddInstrumentation to AddDiagnosticSourceInstrumentation
2. Add AddInstrumentation (this just takes a Function returning the instrumentation. There is no ActivitySourceAdapter. This will be used by any instrumentation which requires lifecycle (create/dispose) managed along with Provider.
3. Mark AddDiagnosticSourceInstrumentation as Internal, and give access to the "special" library instrumentation - HttpClient/SQLClient(.NET Core), AspNetCore, AspNet, GrpcClient(?)
4. Mark DiagnosticSource subscription helper as internal. and give access as above (Or used shared/linked file)
5. For instrumentations in -Contrib repo (ES/MassTransit), rework the implementation to emit ActivitySource.StartActivity, and create own copy of DiagnosticSource subscription helpers.
6. If there is a strong requirement to keep providing the ActivitySourceAdapter mechanism as public API, we can revisit and make them public again after GA release.

The above will cleanup a lot unnecessary public API.

For any other instrumentations not part of this repo:
If its already instrumented with DiagnosticSource, subscribe to those using the DiagnosticSource way of subscribing. In the callbacks, create new activity using ActivitySource. (And ignore the one already created by the library, if any). This does not require ActivitySourceAdapter, and works just like any other ActivitySource based instrumentations. There is a perf hit (from creating a new Activity), but given that the recommendation going forward is to use ActivitySource, this should be a relatively short term thing and is acceptable.

About "special" library - these are libraries which are part of .NET ecosystem itself. Its definitely possible to modify the instrumentations to ignore activity created by the library itself and start a new one using ActivitySource. We need to pursue these library owners (.NET BCL teams, Asp.Net Core teams etc) and ask to move to ActivitySource based instrumentation. Given Asp.Net Core and HttpClient/SqlClient(.NET Core) and among the most used libraries in .NET ecosystem, its probably okay to give them "special" ActivitySourceAdapter, so as to avoid the perf hit when a new activity is created.

[ejsmith]

The contrib project is completely broken when upgrading to OTel 0.8. ActivitySourceAdapter, DiagnosticSourceSubscriber, ListenerHandler, PropertyFetcher have all been made internal. Is the plan that we have to just copy the source into each of these instrumentations?

[AndyPook]

just agreeing with @ejsmith
changing the api may be the right-thing-to-do, but surely there is a series of steps that gets you there without breaking all the things in the meantime?
Making a load of helpers (ie PropertyFetcher) internal is certainly unhelpful for anyone attempting to create custom impl.

On the other hand... thank you for putting the effort on this.
I see 1.0-rc1 is due shortly. Will this be resolved in that version?

[ejsmith]

Yeah, and the thing that is really frustrating to me in projects like this where the community is kind of expected to create an ecosystem of instrumentations, but the root project cheats and makes their internals visible to themselves for their own implementations.

[cijothomas]

Any public API shipped from this repo should be documented and must be supported long term. With this pre-req, we are evaluating all the public APIs exposed, and cutting down anything not required by the spec/not serving the primary intention of this repo.
This includes several extension methods on Activity to high perf alloc-free iterations, easy helpers to subscribe to DiagnosticSource, SemanticConvention helpers etc. They are not required, and are nice-to-have helpers. By default, all of these will be removed from public API before GA.

Its important to do the cleanup before GA, as we cannot make breaking changes to any public API after GA. After GA (i.e next 2-3 weeks), we can evaluate and expose more methods as public, if there is a general need for it with no good alternates. PropertyFetcher may be a good candidate for that.

With the exception of ActivitySourceAdapter, every other methods being removed from public API is not OpenTelemetry specific. ActivitySourceAdapter is tied to this repo, and its not possible for anyone to write its equivalent - so we'll evaluate this after GA (as mentioned earlier).

There are no long term plan of making internalsvisible approach - its required to quickly make progress to GA. They will be removed eventually. OpenTelemetry SDK package should not have any knowledge about other packages, and the current ones are introduced to make GA progress and will be removed.

As to breaking changes affecting customers/other repos- yes, this is expected as we are still beta. Please continue to expect breaking changes until GA release.

[ejsmith]

@cijothomas I understand that you are making a commitment to supporting a public API and I appreciate that. It just raises the barrier of entry for 3rd party instrumentations and makes it harder to create them. I wish you would convert your instrumentations to see what the process is and if it's even possible to create them without making use of internalsvisibleto before you remove them from the rest of the community.

In the meantime, is the best approach to just copy all these files to each project?

[cijothomas]

For DiagnosticSource subscriptions - you are free to copy or write own listeners following .NET's guide of DiagnosticSource : https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/DiagnosticSourceUsersGuide.md

There are instrumentations like Redis, SqlClient(.NET Framework), HttpWebRequest which does not rely on any internals. Also examples are provided for manual instrumentation (https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/examples/MicroserviceExample/Utils/Messaging).

Any 3rd party (and .NET libraries as well, as mentioned in the earlier post) can write instrumentation by just using ActivitySource API alone. It should require no dependency at all to any opentelemetry api or sdk. (With exception of Propagator)
The reference to SDK is only required, if providing an extension method on builder.

[cijothomas]

All DS related classes are made internal, this item can be closed.
Will open separate issue to discuss bringing any useful helpers back to public.