[sdk.logs] Fix the version reported by LogRecord.Logger when using ILogger integration #4635
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4635 +/- ##
=======================================
Coverage 84.94% 84.94%
=======================================
Files 314 313 -1
Lines 12681 12611 -70
=======================================
- Hits 10772 10713 -59
+ Misses 1909 1898 -11
|
src/OpenTelemetry/Sdk.cs
Outdated
| @@ -41,13 +42,20 @@ static Sdk() | |||
| Activity.DefaultIdFormat = ActivityIdFormat.W3C; | |||
| Activity.ForceDefaultIdFormat = true; | |||
| SelfDiagnostics.EnsureInitialized(); | |||
|
|
|||
| var sdkVersion = typeof(Sdk).Assembly.GetCustomAttribute<AssemblyFileVersionAttribute>()?.Version; | |||
There was a problem hiding this comment.
Does this include pre-release info? We were including that https://github.com/open-telemetry/opentelemetry-dotnet/pull/4635/files#diff-82711f0e4129a4bdcd2306a09e8ba95ca230d6b911561e8f38461aa242c7b954:~:text=%7D-,private%20static%20string%20GetAssemblyInformationalVersion(),%7D,-%7D
There was a problem hiding this comment.
Restored! Sorry about that
There was a problem hiding this comment.
How do we test this? The current test seems to be insufficient.
There was a problem hiding this comment.
To be fully robust probably needs an integration test that looks at the actual dll's AssemblyInformationalVersionAttribute spit out by the build system? Don't want to tackle that on this PR but I added a unit test that covers the parse logic.
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets a value indicating whether instrumentation is suppressed (disabled). | ||
| /// </summary> | ||
| public static bool SuppressInstrumentation => SuppressInstrumentationScope.IsSuppressed; | ||
|
|
||
| internal static string InformationalVersion { get; } |
There was a problem hiding this comment.
| internal static string InformationalVersion { get; } | |
| internal static readonly string InformationalVersion; |
There was a problem hiding this comment.
I'm not opposed to this, but why? 🤣 A property with just a getter ({ get; }) is readonly for the backing generated by the compiler IIRC.
There was a problem hiding this comment.
but why?
Just because it's more obvious and intuitive when you use readonly field 😀
Relates to #4433
Changes
AssemblyInformationalVersionAttribute(eg1.5.0-alpha.1) instead ofAssemblyVersion(eg1.0.0.0).Merge requirement checklist