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

Don't bundle instrumentation packages in distro #250

Merged
merged 11 commits into from
Sep 20, 2022

Conversation

MikeGoldsmith
Copy link
Contributor

@MikeGoldsmith MikeGoldsmith commented Sep 15, 2022

Which problem is this PR solving?

Bundling instrumentation within the distro has brought compatibility difficulties, particularly around ASP.NET, ASP.NET Core, Blazor and Unity. This is because these each expect to run on a slightly different runtime / set of APIs. In addition, bundling instrumentation packages in the distro also inflates the size of dependenceis even when an app does not wish to use a given instrumentation package.

A second issue is that any user of the distro that wishes to use instrumentation that the distro did not bundle, was not easily supported. The preferred way to configured the distro was to call AddHoneycomb on the IServiceCollection, but this obfuscated the calls to configure the OpenTelemetry SDK and did not support providing additional instrumentation.

The third issue is that because the instrumentation packages are in prerelease, we would not be able to release an official (1.0.0) version of the distro because it could not include prerelease dependencies.

Short description of the changes

  • removes instrumentation from the distro, including configuration settings in HoneycombOptions and tests
  • removes IServiceCollection extensions, the preferred solution is to use TracerBuilderExtensions instead
  • updates the examples that use instrumentation to install and register instrumentation directly
  • Add IConfigution overload to TracerBuilderExtensions
  • make adding the baggage span processor and deterministic sampler configurable
  • allow adding the baggage span processor, deterministic sampler and otlp exporter independently of using the generic AddHoneycomb call

@MikeGoldsmith MikeGoldsmith added type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible. labels Sep 15, 2022
@MikeGoldsmith MikeGoldsmith requested a review from a team September 15, 2022 12:37
@MikeGoldsmith MikeGoldsmith self-assigned this Sep 15, 2022
@MikeGoldsmith MikeGoldsmith force-pushed the mike/reorganise-packages branch from 8af215d to 3e0ceab Compare September 15, 2022 13:17
@cartermp
Copy link
Member

I don't know if we have a ton of people relying on it, but the need to wire up the baggage processing in the aspnetcore instrumentation does introduce more friction in getting beeline-like functionality that came automatically. I think we should try to figure out a way for people to not have to introduce that code when they wire up aspnet instrumentation (perhaps not as a part of this PR just so we can progress).

Copy link
Member

@cartermp cartermp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this prior to our chat, and there's been some more updates but overall I think this looks great! We can iterate on some finer points if we need to later.

@MikeGoldsmith MikeGoldsmith merged commit a954d12 into main Sep 20, 2022
@MikeGoldsmith MikeGoldsmith deleted the mike/reorganise-packages branch September 20, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More modular instrumentation config
2 participants