Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main open-telemetry/opentelemetry-dotnet#1624 +/- ##
==========================================
- Coverage 73.91% 73.82% -0.10%
==========================================
Files 267 263 -4
Lines 9615 9783 +168
==========================================
+ Hits 7107 7222 +115
- Misses 2508 2561 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
I will be happy to propagate to main repository if we have agree on the naming and values. |
github-actions
bot
assigned CodeBlanch, ejsmith, fred2u, maldago, Oberon00, pcwiese, ppittle, rypdal and srprash
|
Why does the ActivitySource version need to match the package version? |
This would probably be considered a breaking change for the stable packages. |
|
reporting nuget version looks better than assembly version which is 1.0.0.0 always, but i wonder if there was any guidance on this in terms of using nuget version.. Spec says library version as an example, so it is probably good to use nuget version.
I am not sure how would this be a breaking change? It is not changing the semantics on the telemetry itself. Also there is no way to enable/disable based on Version today in the SDK. Ofcourse it should be possible in collector possibly, but still this does not look like breaking change. |
|
Just the heads up. This will work well for libraries that create their own ActivitySource. It won't work for libraries that are simply enriching the activity created by respective frameworks. e.g. So there will still be some inconsistency/gaps. |
I was thinking about its implications for someone using a custom What problem are we trying to solve though? Who is using ActivitySource version and for what purpose? Also, as @vishweshbankwar pointed out we do not have any control over the libraries that natively support |
Got it! |
Good point. Same challenges for "schema_url" too (not yet supported in .NET)! |
I would not say that inconsistency. As a follow up to this (and main reposiroty) we should start recommending what to put as a version. For most cases it should be not assembly version, as it typically is |
|
Updated PR to include the same version for the meters (together with the PR description). |
Shouldn't we fix the assembly version then? |
It is against this recommendation: https://learn.microsoft.com/en-us/dotnet/standard/library-guidance/versioning#assembly-version |
| @@ -21,8 +20,7 @@ public static class AWSLambdaWrapper | |||
| { | |||
| internal const string ActivitySourceName = "OpenTelemetry.Instrumentation.AWSLambda"; | |||
|
|
|||
| private static readonly string Version = typeof(AWSLambdaWrapper).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()!.InformationalVersion.Split('+')[0]; | |||
| private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, Version); | |||
| private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>()); | |||
There was a problem hiding this comment.
Any particular reason for changing the type from which we query the assembly? It should not make a difference, but the minimal change would have been:
| private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>()); | |
| private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaWrapper>()); |
There was a problem hiding this comment.
I have started with this :)
`OpenTelemetry.Instrumentation.AWSLambda.AWSLambdaWrapper': static class type cannot be used as type arguments
| { | ||
| public static string GetVersion<T>() | ||
| { | ||
| return typeof(T).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()!.InformationalVersion.Split('+')[0]; |
There was a problem hiding this comment.
Since we are using !. here, would it be better to use any fallback, or maybe a ?. chain?
There was a problem hiding this comment.
I do not see any case when the null value can happen in any of library.
As long as we are using MinVer'sioning it should be safe to use this construction.
|
@Kielek Would it be possible for you to take this to the OTel community meeting? If you cannot make it to the meeting, @vishweshbankwar can you help? |
Users are not required to provide a version. The spec already says that version is optional. I don't think there is any need for us to recommend what value to use for it.
How are users using ActivitySource version for debugging purposes? |
From observability vendors perspective, if this value is associated with spans, we can easily check what version of instrumentation is used. It is valid especially if customers are using library instrumentations. Based in such data, we can recommend some steps, such us upgrade if fixes are in newer versions. |
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| using System.Reflection; |
There was a problem hiding this comment.
naïve question: Does this have any implication for AOT? Does AOT support getting assembly info?
There was a problem hiding this comment.
I do not see any compilation warnings. Some instrumentation packages are included into AOT app.
Got it! Agree that assembly version won't be able to give such breakdowns.! (the special set of instrumentations where scope is owned by one component, but actual filling of data is done by another won't work as already pointed out, but that is also a temporary state, assuming these libraries will eventually natively produce telemetry without the need of a instrumentation library, like already done for metrics in some cases.) |
Not really. The same library may produce different versions, e.g. for HTTP depending on settings. Also, this use case is perfectly suited for the schema URL that is reported with each scope. |
There was a problem hiding this comment.
LGTM: There is still open question on how to handle the version when instrumentations do not create the ActivitySource for the activities they enrich. None of the libraries here falls under that category.
@Kielek - We could open issues for the special cases in their respective repos.
|
@vishweshbankwar, I created: #5497. Will merge this one shortly (when the CI will finished). |
Fixes: issue when it is hard to determine what is the instrumentation library version used in end-user project.
Most of packages just reporte 1.0.0.0 (Assembly version).
MinVer is setting more user friendly value in
AssemblyInformationalVersionAttributeattribute in following format:1.7.2-alpha.0.39+69cc79aed00a37c103b9e46488e2001db959a1ab.It was previously set correctly only in one package.
Changes
Instrumentation spans - set ActivitySource.Version and Meter.Version to NuGet package version
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes[ ] Design discussion issue #[ ] Changes in public API reviewed