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

Automatic netfx assembly redirection #1825

Merged
merged 11 commits into from
Dec 21, 2022

Conversation

pjanotti
Copy link
Contributor

Why

To automate instrumentation of .NET Framework without requiring binding redirection and reducing the cases that adding dependencies to the projects are needed.

Fixes #1646

What

  1. During the build, build a list of the assemblies shipped in the netfx folder and use it to generate a map of redirections to be used by the native profiler in _WIN32 compilation.
  2. When loading a module the CLR profiler updates any assembly present in the list to use the higher version: either from the application reference of from the one available on netfx.

The risk is that dependencies are changed at runtime and it is possible that issues only show up when certain code paths are executed. A much safer path would be to have the application reference a NuGet package with all the dependencies required by automatic instrumentation.

To minimize the risk of downgrading packages used by the application, details here, we need the work from #1824 and #1823.

Tests

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests: by removal of the previous workarounds.

@pjanotti pjanotti requested a review from a team December 15, 2022 21:21
@github-actions github-actions bot requested a review from theletterf December 15, 2022 21:23
This reverts commit e291eb8.

We need some extra work to automate this process and also a friendly way
to register assemblies in the GAC.
@pjanotti pjanotti force-pushed the net-fx-auto-redirect branch from 5c2e20a to f6fb7c7 Compare December 15, 2022 21:35
Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

Briefly checked. I will revisit it on Monday.

I left only some nit comments.

build/Build.cs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@pjanotti
Copy link
Contributor Author

I'm having second thoughts about having a static list of versions: we could look at startup for the assemblies shipped with the app to update the version of the redirection. Anyway, I would prefer to address that in a separate PR.

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

LGTM, on comment to review.

@Kielek
Copy link
Contributor

Kielek commented Dec 20, 2022

I'm having second thoughts about having a static list of versions: we could look at startup for the assemblies shipped with the app to update the version of the redirection. Anyway, I would prefer to address that in a separate PR.

I suppose that it will increase cold start time. Additional measurements are needed.

docs/config.md Outdated Show resolved Hide resolved
@pjanotti pjanotti enabled auto-merge (squash) December 21, 2022 19:02
@pjanotti pjanotti merged commit de86993 into open-telemetry:main Dec 21, 2022
@pjanotti pjanotti deleted the net-fx-auto-redirect branch December 21, 2022 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing expected assembly when instrumenting .NET Framework apps
4 participants