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

ActivitySourceAdapter #1580

Closed
cijothomas opened this issue Nov 18, 2020 · 5 comments
Closed

ActivitySourceAdapter #1580

cijothomas opened this issue Nov 18, 2020 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@cijothomas
Copy link
Member

As discussed in #1437 , we have made DS listener helpers are internal.

Opening a new issue to track one undecided item from the original issue:
ActivitySourceAdapter - was developed to work around the fact that some libraries are already instrumented with DS and create activity without ActivitySource. ActivitySourceAdapter provided a way to avoid throwing away the activity and creating a fresh one using ActivitySource. The ActivitySourceAdapter is internal, and workaround this limitation by "special" InternalsVisibleTo attribute, which should be removed long term.

The first thing to try out - pursue those libraries from .NET team, and see if we can modify them to use ActivitySource API to construct Activity, without breaking existing customers. This would avoid us requiring the special magic of ActivitySource adapter.

If this is not feasible, expose ActivitySourceAdapter public, and let all instrumentations use it.
Or
Modify these instrumentation libraries to throw the original activity away, and create new one. There is a perf penality to it.

@cijothomas
Copy link
Member Author

Modify these instrumentation libraries to throw the original activity away, and create new one. There is a perf penality to it.

Based on experiments with MassTransit, it looks the the above approach may not work for some libraries.
Given this, we need to make ActivitySourceAdapter public again.

(irrespective of this, still working with few other library owners to upgrade to ActivitySource for long term)

@alanwest
Copy link
Member

alanwest commented Jan 8, 2021

Given this, we need to make ActivitySourceAdapter public again.

If there ends up being a need to make it public then it might make sense to move this to a project of its own - something like OpenTelemetry.Extensions.Instrumentation. Right now it resides in the SDK project, and of course being a .NET specific thing is not part of the SDK specification.

Another thought would be to move a copy of ActivitySourceAdapter to the contrib repository that any of the instrumentation there could use.

@CodeBlanch
Copy link
Member

Kind of on the fence about it being public or in another assembly as @alanwest suggested. I think I'm leaning towards separate assembly, in the main repo. Maybe then we could also promote some of the other stuff that is helpful to instrumentation authors there (ex DiagnosticSourceSubscriber \ DiagnosticSourceListener)?

@cijothomas
Copy link
Member Author

DiagnosticSourceSubscriber,etc are pure .NET and not OpenTelemetry specific, so its not required to be exposed as public. (exposing it'll make writing instrumentations a bit easier, but they may be copy-pasted as well..

ActivitySourceAdapter is something no one can build on their own, so we have to expose it. (the alternate idea of throwing away and recreating activity is not going to work in some scenarios like MT).

Another thought would be to move a copy of ActivitySourceAdapter to the contrib repository that any of the instrumentation there could use.

Not sure if it is feasible. As constructing it requires access to processors, which only TracerProviderBuilder has. Its currently enabled via the below internal API.
https://github.com/open-telemetry/opentelemetry-dotnet/blob/master/src/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs#L151

@cijothomas
Copy link
Member Author

#1836 made TracerProviderSdk natively support legacy activities, eliminating the need for activitysourceadapter. Closing this issue now.

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

No branches or pull requests

3 participants