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

Resolve file loading issues originating from Runtime store libraries #3343

Conversation

rajkumar-rangaraj
Copy link
Contributor

Why

Fixes #3168, #3075
Follow up #3187

Steps to recreate an issue:: Adding the below package reference to Examples.AspNetCoreMvc project for net6.0 target it will reproduce crash issue.

<PackageReference Include="Microsoft.Extensions.Configuration" Version="6.0.1" />

What

The .NET loader takes control when the assembly load fails. If an assembly is part of the runtime store, we load it in a different context. This approach prevents crashes and allows ILogger data collection.

Rule engine needs to be turned off to test this approach. Diagnostics rule has a backoff logic.

Tests

Existing CI

Checklist

  • [ ] CHANGELOG.md is updated.
  • [ ] Documentation is updated.
  • [ ] New features are covered by tests.

@rajkumar-rangaraj rajkumar-rangaraj requested a review from a team March 29, 2024 06:19
@rajkumar-rangaraj
Copy link
Contributor Author

@open-telemetry/dotnet-instrumentation-approvers Please review and test the behavior with Examples app in our repo. This approach resolves the crash issues that occur due to runtime store conflicts. Do set OTEL_DOTNET_AUTO_RULE_ENGINE_ENABLED to false to test it.

@Kielek
Copy link
Contributor

Kielek commented Mar 29, 2024

@rajkumar-rangaraj, could you please add CHANGELOG entry for this fix?

I suppose it could convince some users to upgrade.

Copy link
Contributor

@pjanotti pjanotti left a comment

Choose a reason for hiding this comment

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

Very nice @rajkumar-rangaraj!

I will give a spin later today to validate some cases, but, at first LGTM. Per @Kielek comment a note on changelog will be good.

@rajkumar-rangaraj
Copy link
Contributor Author

@rajkumar-rangaraj, could you please add CHANGELOG entry for this fix?

I suppose it could convince some users to upgrade.

@Kielek Added changelog, please take a look.

@rajkumar-rangaraj rajkumar-rangaraj merged commit 2f8dc0a into open-telemetry:main Apr 1, 2024
41 checks passed
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.

Version mismatch
4 participants