[Instrumentation.GrpNetClient, Instrumentation.SqlCliet] set ActivitySource.Version to NuGet package version

[Kielek]

Towards open-telemetry/opentelemetry-dotnet-contrib#1758
Design discussion issue open-telemetry/opentelemetry-dotnet-contrib#1624

Changes

Fist step to cover all packages (packages not mentioned in open-telemetry/opentelemetry-dotnet-contrib#1758) in this repository with consistent way of Instrumentation Scope version.

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• [ ] Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• [ ] Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.56%. Comparing base (6250307) to head (bd3b88b).
Report is 172 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    open-telemetry/opentelemetry-dotnet#5498      +/-   ##
==========================================
+ Coverage   83.38%   85.56%   +2.18%     
==========================================
  Files         297      289       -8     
  Lines       12531    12583      +52     
==========================================
+ Hits        10449    10767     +318     
+ Misses       2082     1816     -266     

Flag	Coverage Δ
unittests	?
unittests-Instrumentation-Experimental	24.14% <100.00%> (?)
unittests-Instrumentation-Stable	24.14% <100.00%> (?)
unittests-Solution-Experimental	85.51% <100.00%> (?)
unittests-Solution-Stable	85.52% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ent/Implementation/GrpcClientDiagnosticListener.cs	76.19% <100.00%> (-2.39%)	⬇️
...qlClient/Implementation/SqlActivitySourceHelper.cs	100.00% <100.00%> (ø)
src/OpenTelemetry/Sdk.cs	100.00% <100.00%> (ø)

... and 65 files with indirect coverage changes

[reyang]

The PR title and changelog both say this is NuGet package version, the code here doesn't seem to be NuGet version (I guess there is some assumption that the portion of assembly version would be the same as the package version, not sure if that's something we can rely on though), could you explain how this would work?

[Kielek]

I am not sure if you have chance to review: open-telemetry/opentelemetry-dotnet-contrib#1624. I will try to description behavior once more time.

1. Before changes, InstrumentationScope.Version was reported as Assmebly.Version.
2. Assembly.Version by this recommendation typically is set to nonzero.0.0.0. In all instrumentation packages it return 1.0.0.0. In general it is almost useless value, as we have tons of different behaviors in such scope.
3. Both, this repository and contrib are using MinVer for versioning libraries and nuget packages.
4. MinVer together with SourceLink (commit part) produces for commit 33d5521 for SqlClient instrumentation library following values: (last release/sql tag was set to 1.7.0-beta.1)
   • NuGet version: 1.7.0-beta.1.86
   • AssemblyInformationalVersionAttribute 1.7.0-beta.1.86+33d5521a73e881ac59d4bf1213765270ec2422ff (this is in format {nuget-version}-{commithash}.
5. NuGet version is in sync with all other versions

IMO, for end users, describing new value as a NuGet version is the easiest option. Relaying on AssemblyInformationalVersionAttribute is more problematic.

[reyang]

Need some clarification here https://github.com/open-telemetry/opentelemetry-dotnet/pull/5498/files#r1550305840

Blocking concern addressed

[CodeBlanch]

@Kielek We have similar logic here:

opentelemetry-dotnet/src/OpenTelemetry/Sdk.cs

Lines 34 to 35 in 3f4c73a

	var assemblyInformationalVersion = typeof(Sdk).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
	InformationalVersion = ParseAssemblyInformationalVersion(assemblyInformationalVersion);

opentelemetry-dotnet/src/OpenTelemetry/Sdk.cs

Line 114 in 3f4c73a

internal static string ParseAssemblyInformationalVersion(string? informationalVersion)

Also here but it seems a little different:

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

Line 232 in 3f4c73a

var assemblyVersion = typeof(OtlpExporterOptions).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>();

This new helper class doesn't really have anything to do with instrumentation scopes. Could we perhaps rename it something like AssemblyVersionHelper and refactor the SDK version stuff (maybe OtlpExporter stuff too) so that everything is using the same code for versioning?

Doesn't have to be on this PR.

The API could also be an extension method. Something like...

internal static class AssemblyVersionExtensions
{
    public static string GetPackageVersion(this Assembly assembly)
    {
        // not shown
    }
}

In the current signature you pass a type to use to lookup the assembly. I don't think that is immediately obvious IMO passing the assembly to use is nicer/more explicit.

[Kielek]

Most of the changes covered in 9f5f5bf and d8f9a1a.

I think that changes in OTLP exported, related to dropping hash commit, deservers CHANGELOG entry.
It will bring as a bit closer to recommendation, to put there just version, without any additional data such as commit hash.
I will create follow up PR, when this one will be merged.

[vishweshbankwar]

@Kielek - What will change with respect to OTLP exporter? Could you elaborate?

[Kielek]

User agent. Now it contains haskcommit.

Before changes OTel-OTLP-Exporter-Dotnet/version+hashcommit
After-changes OTel-OTLP-Exporter-Dotnet/version

[CodeBlanch]

Couple of nits but LGTM.