[AOT] Resolve ConfigurationExtensions and EventSource warnings#4534
Merged
utpilla merged 5 commits intoopen-telemetry:mainfrom Jun 2, 2023
Merged
[AOT] Resolve ConfigurationExtensions and EventSource warnings#4534utpilla merged 5 commits intoopen-telemetry:mainfrom
utpilla merged 5 commits intoopen-telemetry:mainfrom
Conversation
- ConfigurationBinder.GetValue uses Reflection to bind IConfiguration values to strongly typed objects. ConfigurationExtensions.TryGetStringValue was using ConfigurationBinder to get a string value from IConfiguration, which is causing a warning. However, IConfiguration values are already strings, so this is unnecessary. It is also not performant because calling ConfigurationBinder allocates objects and uses TypeDescriptor. - Additionally, suppress 2 EventSource warnings while I'm making changes. Contributes to open-telemetry#3429
eerhardt
commented
Jun 1, 2023
src/OpenTelemetry.Instrumentation.AspNetCore/OpenTelemetry.Instrumentation.AspNetCore.csproj
Show resolved
Hide resolved
Contributor
|
Fixing these configuration warnings would result in publish AotTestApp to be successful? From CI: |
Contributor
Author
Isn't this the problem you are addressing with https://github.com/open-telemetry/opentelemetry-dotnet/pull/4460/files#diff-982b4ef6e39d21d7e5eac21b9a818d363c9bfe68b62af7e2ccd922ea0697a9baR84? I don't see how resolving these warnings would make the publish to start timing out now. |
Yun-Ting
approved these changes
Jun 1, 2023
utpilla
reviewed
Jun 1, 2023
utpilla
reviewed
Jun 1, 2023
...nTelemetry.Instrumentation.AspNetCore/Implementation/AspNetCoreInstrumentationEventSource.cs
Show resolved
Hide resolved
eerhardt
commented
Jun 1, 2023
utpilla
approved these changes
Jun 1, 2023
vitek-karas
approved these changes
Jun 2, 2023
reyang
approved these changes
Jun 2, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4534 +/- ##
==========================================
- Coverage 85.61% 85.58% -0.04%
==========================================
Files 320 320
Lines 12610 12610
==========================================
- Hits 10796 10792 -4
- Misses 1814 1818 +4
|
mfogliatto
pushed a commit
to mfogliatto/opentelemetry-dotnet
that referenced
this pull request
Jun 10, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ConfigurationBinder.GetValue uses Reflection to bind IConfiguration values to strongly typed objects. ConfigurationExtensions.TryGetStringValue was using ConfigurationBinder to get a string value from IConfiguration, which is causing a warning. However, IConfiguration values are already strings, so this is unnecessary. It is also not performant because calling ConfigurationBinder allocates objects and uses TypeDescriptor.
Additionally, suppress 2 EventSource warnings while I'm making changes.
Contributes to #3429
cc @Yun-Ting @vitek-karas