Skip to content

Conversation

@xiang17
Copy link
Contributor

@xiang17 xiang17 commented Apr 6, 2023

Why

Follow up of #2405 for DS rule
For issue #2392

What

This PR adds a version check rule for System.Diagnostics.DiagnosticSource. It will back off the auto instrumentation in case a System.Diagnostics.DiagnosticSource is included that can cause the auto instrumentation to crash.

Automated tests are to be added as follow up.

Tests

Unit tests

Checklist

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

@xiang17 xiang17 requested a review from a team April 6, 2023 01:05
@xiang17
Copy link
Contributor Author

xiang17 commented Apr 7, 2023

Removed unit tests. It's probably better to test in integration tests, because most of the logic are checking the environment (the Type.GetType() call and version check of DiagnosticSource that's in tracer-home\net folder) outside the class anyways, except the less than check.

For example, checking the DiagnosticSource version of the auto instrumentation library depends on checking StartupHook.LoaderAssemblyLocation, which is the tracer-home\net folder. The unit test project OpenTelemetry.AutoInstrumentation.StartupHook.Tests.csproj doesn't have it.

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.

Minor suggestions, but, otherwise LGTM. There should be a follow-up to get the version wanted by auto-instrumentation at build time or from the json created in a separate PR by Raj.

@pjanotti pjanotti enabled auto-merge (squash) April 12, 2023 20:01
@pjanotti pjanotti merged commit 0f15c72 into open-telemetry:main Apr 12, 2023
@xiang17 xiang17 deleted the xiang17/DiagnosticSourceDllVersionCheck branch April 13, 2023 00:16
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.

5 participants