Do we want to have internals visible to opentelemetry-dotnet-contrib projects?

[nmbro]

Question

Do we want to have internals visible to opentelemetry-dotnet-contrib projects?

Additional Context

It could make upgrading opentelemetry-dotnet-contrib easier

See open-telemetry/opentelemetry-dotnet-contrib/issues/49
and open-telemetry/opentelemetry-dotnet-contrib/issues/49

[cijothomas]

There shouldn't be any need of exposing internals. If meeting a common requirement, then we can make it public.

There is a special case about ActivitySourceAdapter #1580. We haven't finalized the plans on it.

[nmbro]

TestExporter might be another special case unless we want to have a Opentelemetry.Testing package, and it might be too little code to really serve any purpose.

[cijothomas]

Can you elaborate why TestExporter is a special case?

[nmbro]

Only that it's only used for testing - and it lives in Opentelemetry.Tests, which is not something I would assume should be packable.

[buvinghausen]

@nmbro making the internals accessible to the contrib projects still doesn't solve the problem for 3rd party or custom instrumentation providers.

@cijothomas from what I can tell the StackExchangeRedis instrumentation is the only one that is operational without internals being made visible to. Is this the template the contrib projects should follow?

[cijothomas]

@buvinghausen No we dont expect to make any internalsvisible permanent. Its done now to unblock GA progress. The only thing which is really tied to OpenTelemetry is ActivitySourceAdapter, which we are working to solve. Things like DiagnosticSourceListeners - are not tied to OpenTelemetry specifics. (one may copy the approach from this repo).

Please check the following doc on how to write instrumentations:
https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/docs/trace/extending-the-sdk#instrumentation-library
Happy to help if any portion is not clear.

[buvinghausen]

@cijothomas thank you for the clarifications. So I finally pulled down the repository and got an understanding of the codebase. Based on your explanation about how the objects in question are not part of the Otel spec my recommendation is break the DiagnosticSourceInstrumentation folder out of the main project and put it into a separate assembly like OpenTelemetry.Instrumentations.Abstractions and then restore the public visibility to things like DiagnosticSourceSubscriber, ListenerHandler, & PropertyFetcher

That would solve the issue of making sure someone doesn't grab something out of the main OpenTelemetry assembly they shouldn't. It still leaves the tracing discussions for the ActivitySourceAdapter solution up to group at large. I believe this to be a nice happy medium so that the instrumentation packages can take a dependency on the abstractions package where things like exporters and spec related items will not have them around.

If you think that is amenable/feasible I'm happy to open a PR and take a stab at it so let me know.

[cijothomas]

separate assembly like OpenTelemetry.Instrumentations.Abstractions and then restore the public visibility to things like DiagnosticSourceSubscriber, ListenerHandler, & PropertyFetcher

There was a similar proposal several months back to separate these into own package like Otel.Instrumentation.Helpers - just like your proposal. But these classes are just regular .NET code, with no connection to OpenTelemetry specs. DiagnosticSource callbacks is just one way of "hijacking" into a library (https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/docs/trace/extending-the-sdk#writing-own-instrumentation-library Step 1).

I agree that exposing these as public packages makes it easier to write own instrumentations, but then its an additional thing to maintain, and not truly serving long term direction (As the guidance is to move away from DS, and move to ActivitySource.). We also need to solve issues like this #1561, if we expose DiagnosticSourceSubscribers public.

Could you try the guidance for writing instrumentation library, and see if it can be done, without requiring access to any internals? The instrumentation should only require a reference to System.Diagnostics.DiagnosticSource package, or OpenTelemetry.API (if need propagators) package. It should involve writing own DiagnosticSource listeners (free to copy from this repo, or follow .NET guidance - https://github.com/dotnet/runtime/blob/master/src/libraries/System.Diagnostics.DiagnosticSource/src/DiagnosticSourceUsersGuide.md), and create new Activity using the ActivitySource API. Neither should require any access to internals.
If you are looking for an example, we can refactor the SqlClientDiagnosticListener, to not use any internals, and still achieve its purpose. (Or refactor some other instrumentation in -contrib repo)

Fate of ActivitySourceAdapter is still undecided. We are working with .NET team to migrate their libraries to use ActivitySource, so that we can totally eliminate the need of ActivitySourceAdapter.

[alexvaluyskiy]

The another option for the contrib packages is to move them back into this repository. And InternalVisibleTo will work for them too.

The problem is the the current version of telemetry can't be used by many users, because it is not cover all instrumentations.
For example, I need EF Core instrumentation, because I'm using PostgreSql. Also I need an instrumentation for a message broker (Azure Service Bus, MassTransit or Kafka). And also I want to write a custom tracer for Microsoft Orleans. Without these instrumentation, it does not show the full picture of tracing.

[cijothomas]

@alexvaluyskiy which particular internal class/api from this repo is required for you to write custom instrumentation for, say, Microsoft Orleans or Kafka?

[buvinghausen]

@alexvaluyskiy take a look at how Jimmy Bogard set up instrumentation for NServiceBus

[alexvaluyskiy]

NServiceBus example is a good choice for custom instrumentation (Kafka, Orleans). But what should I do with MassTransit which is already has an activity? Or with EFCore, which have a DiagnosticListener?

[cijothomas]

@alexvaluyskiy please take a look here : https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/docs/trace/extending-the-sdk#instrumentation-library

It covers the special case of libraries already producing Activity. The solution is not 100% perfect due to perf hit - the ideal way is working with the library authors to switch from DS to ActivitySource. (I'm working to get all Microsoft libraries updated)

[alexvaluyskiy]

@cijothomas I've tried to add a tracing to Microsoft Orleans via ActivitySource and it works great.

But I still don't understand - what to do with current instrumented libraries, which have already provided DiagnosticSource or Activity tracing? Even if all of them will migrate to ActivitySource - it means, that it will be supported only in .NET 6 (Asp.NET Core, SqlClient, EF Core, Grpc, etc) or it will require to migrate to the newest SDK/library (Azure SDK, AWS SDK, MassTransit).

I think, the adapters for DiagnosticSource and Activities still should be present. Probably, not in the main OpenTelemetry package.

There was a similar proposal several months back to separate these into own package like Otel.Instrumentation.Helpers - just like your proposal. But these classes are just regular .NET code, with no connection to OpenTelemetry specs.

I've even tried to copy all this classes (from DiagnosticSourceInstrumentation folder) in my custom instrumentation, but I've failed.
ActivitySourceAdapter can't be copied, because it uses another internal classes.
The extensions for adding these adapters are also internal - link

[cijothomas]

@alexeyzimarev You shouldn't need ActivitySourceAdapter.
You may copy this whole folder : https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/DiagnosticSourceInstrumentation to get DiagnosticSource subscriptions and callbacks.
Create brand new activity using ActivitySource as sibling of the existing activity. Let me know if you need help with trying this - I will be looking at doing this for one of the instrumentation in contrib repo.

(Not opposed to the idea of making all of the above, including ActivitySourceAdapter as a separate instrumentation.helpers package, but want to make sure this is absolutely needed before we make something public.)

[julioct]

@alexeyzimarev @cijothomas I just also went through the painful exercise of trying to bring ActivitySourceAdapter (and several other internal classes) to my code base, without success (the rabbit hole is deep!). I had created a simple set of OpenTelemetry instrumentation classes for MassTransit, but it's all broken with OTel 1.0 RC1. I chatted with the owner of the MassTransit libraries, which already use DiagnosticSource activities, and he is not willing to take a dependency on classes that rely on .NET 5, like ActivitySource, which is fair.

@cijothomas Can you please provide one simple concrete example of how should people write the instrumentation library for an instrumented library that is already using activities via DiagnosticSource? Not hacks like SqlClient, Redis or HttpWebRequest. An actual simple, copy-paste, clean example for all the folks that are in this same boat.

[cijothomas]

@julioct

take a dependency on classes that rely on .NET 5, like ActivitySource, which is fair.

DiagnosticSource improvements are shipped as out-of-band nuget package, and works in all .NET versions (including old ones like .NET452), not just the new .NET5.

[cijothomas]

@julioct Yes, we will work to modify at least one of -contrib repo instrumentation to act as an example.

As mentioned earlier, only thing you may copy paste is https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/DiagnosticSourceInstrumentation. ActivitySourceAdapter is definitely a hack - we are undecided on its fate, but leaning towards getting rid of it, wherever possible.

The last section of this doc (https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/docs/trace/extending-the-sdk#instrumentation-library) does describe what to do if an activity is already created outside of ActivitySource. Agree it does not provide actual example. Did you follow this guide ? If yes, could you elaborate what specific issues are encountered, so we can revisit this section.

[julioct]

@julioct Yes, we will work to modify at least one of -contrib repo instrumentation to act as an example.

As mentioned earlier, only thing you may copy paste is https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/DiagnosticSourceInstrumentation. ActivitySourceAdapter is definitely a hack - we are undecided on its fate, but leaning towards getting rid of it, wherever possible.

The last section of this doc (https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/docs/trace/extending-the-sdk#instrumentation-library) does describe what to do if an activity is already created outside of ActivitySource. Agree it does not provide actual example. Did you follow this guide ? If yes, could you elaborate what specific issues are encountered, so we can revisit this section.

@cijothomas Thanks for the quick reply! Yes, I think you refer to this portion:
"There is a special case for libraries which are already instrumented with Activity, but using the DiagnosticSource method. These libraries already emit activities, but it may not conform to the OpenTelemetry semantic conventions. Also, as these libraries do not use ActivitySource to create Activity, they cannot be simply subscribed to. In such cases, the instrumentation library should subscribe to the DiagnosticSource events from the instrumented library, and in turn produce new activity using ActivitySource. This new activity must be created as a sibling of the activity already produced by the library. i.e the new activity must have the same parent as the original activity. Some common examples of such libraries include Asp.Net, Asp.Net Core, HttpClient (.NET Core). Instrumentation libraries for these are already provided in this repo."

I appreciate the guidance, but please show me the code :) That section basically points me to the instrumentation libraries that today use ActivitySourceAdapter. I'd like to know how to do this cleanly without ActivitySourceAdapter for an instrumented library that is already using DiagnosticSource.

[cijothomas]

@julioct

The simplified version of actual steps would be:

1. "Hijack" - Naturally, this is DiagnosticSource callbacks provided by the library.
2. Get the Activity already created by the library (i.e Activity.Current)
3. Get its parent context, if any.
4. use ActivitySource to start a new activity, with the parent from step3, and correct ActivityKind.
5. stop the new activity when done.
6. The extension method for AddXYZInstrumentation() should call builder.AddSource("ActivitySourceName"), for the ActivitySource created in step 4.

@eddynaka is working to get an actual example code for this.

[alexeyzimarev]

@alexeyzimarev You shouldn't need ActivitySourceAdapter.

I guess you wanted to mention @alexvaluyskiy :)

[cijothomas]

Oh yes! Thanks for correcting :)

[eddynaka]

Hello,

just created the PR as draft so you can see what we did to replace the ActivitySourceAdapter.

[julioct]

Hello,

just created the PR as draft so you can see what we did to replace the ActivitySourceAdapter.

That was fast, thanks @eddynaka!

[eddynaka]

Hello,
just created the PR as draft so you can see what we did to replace the ActivitySourceAdapter.

That was fast, thanks @eddynaka!

No problem at all! Glad to help. The build is failing because I didn't update the public API yet. Will do soon. But, the overall idea is there. :)

[julioct]

Btw, just wanted to mention I have a fully working solution based on Eddy's PR. Thanks again @cijothomas and @eddynaka!

[eddynaka]

Btw, just wanted to mention I have a fully working solution based on Eddy's PR. Thanks again @cijothomas and @eddynaka!

@julioct , nice to know that it helped! I saw that my PR tests are failing. Will have to take a closer look to the GRPC, since they are together...

[cijothomas]

As mentioned before, this approach is not the ideal one as we are allocating an additional activity. But this is something we should live with, until an alternate can be made. As a reminder, the root of the issue is that some libraries create Activity, without using ActivitySource API, and those activities bypass samplers/processors, have default Kind, and have "empty" ActivitySource. The proper fix would be to change those libraries to leverage ActivitySource. Migrating to ActivitySource is probably a bigger work item, but replacing just new Activity().Start with ActivitySource.Start() would be minimal work, and can be done in such a way to ensure its fully backward compatible. (I am working to try this in AspNet. Once I have a prototype, will try to encourage other libraries to do the same)

[bravecobra]

Could we have a (temporary) working solution in the meantime? Every contrib project is broken now.

[cijothomas]

Could we have a (temporary) working solution in the meantime? Every contrib project is broken now.

Agree this is not a great state to be in. We are still discussing and finalizing the status of -contrib repo. (its not officially defined what goes in -contrib vs what goes in the main repo. Also who should maintain the repo). I expect this would be finalized early next year.

Meanwhile - the approach for fixing the instrumentations are demo-ed in the linked PR. Depending on people's availability, this approach can be leveraged to fix the contrib repo.

[joshclark]

As mentioned earlier, only thing you may copy paste is https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/DiagnosticSourceInstrumentation.

FYI, this won't work. DiagnosticSourceListener uses SuppressInstrumentationScope.IncrementIfTriggered() which is internal.

[cijothomas]

As mentioned earlier, only thing you may copy paste is https://github.com/open-telemetry/opentelemetry-dotnet/tree/master/src/OpenTelemetry/DiagnosticSourceInstrumentation.

FYI, this won't work. DiagnosticSourceListener uses SuppressInstrumentationScope.IncrementIfTriggered() which is internal.

They could be potentially exposed as public.

[mikedoug]

Are there any plans and/or timelines for resolving this?

As a user of your amazing project, very little makes sense in this rigid stance of keeping things private and not exposing the items that you need even for your own contrib code to work much less the code for other libraries. I get the need to help move new standards forward and encourage their adoption, and I get keeping your supported API footprint small -- but you're taking a great project and seriously restricting its usefulness by doing so. Also, .NET 5 and this new ActivitySource just came out -- forcibly requiring every library in existence to adopt it immediately is just a bad position.

As a user I'm looking to understand what the plan is moving forward -- because right now we're stuck between a rock and a hard place -- and that's a bad place to be. We're looking and hoping for a little light! Thanks!

[cijothomas]

@mikedoug
If you mean do we have plans to make internals visible to -contrib - No this is not planned. (Infact the plan to get rid of the "special" access to even the projects from inside this repo itself.)

ActivitySourceAdapter, being special case, is decided to be made public (#1580). This will be done as soon as 1.0 is released. (instrumentation projects are going to be released after API/SDK only due to spec requirements, so current focus is to get API/SDK releases soon)

Regd. ETA for updating -contrib repo and start shipping packages from it - No ETA on it. A lot depends on this issue (open-telemetry/opentelemetry-dotnet-contrib#57) which is proposing how best to organize the contrib repo for long term sustainability.

[mikedoug]

@cijothomas I mean specifically "whatever it takes to get the contrib working again for EF Core and Mass Transit". If that's just making ActivitySourceAdapter public, then that is perfect. The sooner that can happen the sooner we (and probably many others that are in love with your project) can move forward with our distributed systems. I understand that there is a desire to reorganize the contrib, but from a user's standpoint it would be nice (should the work not be enormous) if the existing code could be returned to a working state while that transition is worked through. It's easy enough to switch, as a consumer, to new nuggets in the future while being able to start operating with the existing work today.

Just so we are clear: I mean no disrespect in my questioning here either -- we all greatly admire and appreciate the work everybody has done in this project. Thanks to an infinite degree!

[cijothomas]

The sooner that can happen

Yes I understand! Just need to finish 1.0 release of API/SDK/Exporters, and then address the instrumentation issues, including the ones in contrib. As per current plan, the 1.0 release occurs next week, and right after that, we can start fixing the instrumentations. Thanks for being patient.

[mikedoug]

That is awesome! Hearing "next week" really makes me happy. Thank you for taking the time to respond! I'm going to leave you alone now. :)