Skip to content

Conversation

@eerhardt
Copy link
Contributor

@eerhardt eerhardt commented Jun 1, 2023

  • 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

- 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 eerhardt requested a review from a team June 1, 2023 14:48
@Yun-Ting
Copy link
Contributor

Yun-Ting commented Jun 1, 2023

Fixing these configuration warnings would result in publish AotTestApp to be successful?

From CI:

2023-06-01T15:24:20.5693349Z   Failed OpenTelemetry.AotCompatibility.Tests.AotCompatibilityTests.EnsureAotCompatibility [3 m]
2023-06-01T15:24:20.5694217Z   Error Message:
2023-06-01T15:24:20.5696849Z    dotnet publish command timed out after 180 seconds.
2023-06-01T15:24:20.5698315Z Expected: True
2023-06-01T15:24:20.5698950Z Actual:   False
2023-06-01T15:24:20.5700446Z   Stack Trace:
2023-06-01T15:24:20.5702203Z      at OpenTelemetry.AotCompatibility.Tests.AotCompatibilityTests.EnsureAotCompatibility() in D:\a\opentelemetry-dotnet\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.Tests\AotCompatibilityTests.cs:line 84
2023-06-01T15:24:20.5703611Z    at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
2023-06-01T15:24:20.5704506Z    at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

@eerhardt
Copy link
Contributor Author

eerhardt commented Jun 1, 2023

Fixing these configuration warnings would result in publish AotTestApp to be successful?

From CI:

2023-06-01T15:24:20.5693349Z   Failed OpenTelemetry.AotCompatibility.Tests.AotCompatibilityTests.EnsureAotCompatibility [3 m]
2023-06-01T15:24:20.5694217Z   Error Message:
2023-06-01T15:24:20.5696849Z    dotnet publish command timed out after 180 seconds.
2023-06-01T15:24:20.5698315Z Expected: True
2023-06-01T15:24:20.5698950Z Actual:   False
2023-06-01T15:24:20.5700446Z   Stack Trace:
2023-06-01T15:24:20.5702203Z      at OpenTelemetry.AotCompatibility.Tests.AotCompatibilityTests.EnsureAotCompatibility() in D:\a\opentelemetry-dotnet\opentelemetry-dotnet\test\OpenTelemetry.AotCompatibility.Tests\AotCompatibilityTests.cs:line 84
2023-06-01T15:24:20.5703611Z    at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
2023-06-01T15:24:20.5704506Z    at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

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.

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Merging #4534 (ca75280) into main (9b5c483) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...ementation/AspNetCoreInstrumentationEventSource.cs 80.00% <ø> (ø)
...emetry/Internal/Options/ConfigurationExtensions.cs 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

@utpilla utpilla merged commit e67d44e into open-telemetry:main Jun 2, 2023
@eerhardt eerhardt deleted the FixConfigBinder branch June 2, 2023 19:43
mfogliatto pushed a commit to mfogliatto/opentelemetry-dotnet that referenced this pull request Jun 10, 2023
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