Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrumentation spans and metrcis - set ActivitySource/Meter.Version to NuGet package version #1624

Merged
merged 11 commits into from
Apr 2, 2024

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Mar 25, 2024

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 AssemblyInformationalVersionAttribute attribute 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:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • [ ] Design discussion issue #
  • [ ] Changes in public API reviewed

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.82%. Comparing base (71655ce) to head (8d9a225).
Report is 175 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
unittests-Exporter.Geneva 59.81% <ø> (?)
unittests-Exporter.OneCollector 89.72% <ø> (?)
unittests-Extensions 82.75% <ø> (?)
unittests-Instrumentation.AspNet 77.55% <100.00%> (?)
unittests-Instrumentation.EventCounters 75.92% <100.00%> (?)
unittests-Instrumentation.Owin 85.61% <100.00%> (?)
unittests-Instrumentation.Process 100.00% <100.00%> (?)
unittests-Instrumentation.Runtime 100.00% <100.00%> (?)
unittests-Instrumentation.StackExchangeRedis 70.89% <100.00%> (?)
unittests-Instrumentation.Wcf 78.44% <100.00%> (?)
unittests-PersistentStorage 65.78% <ø> (?)
unittests-ResourceDetectors.Azure 81.53% <ø> (?)
unittests-ResourceDetectors.Host 40.00% <ø> (?)
unittests-ResourceDetectors.Process 83.21% <ø> (?)
unittests-ResourceDetectors.ProcessRuntime 76.08% <ø> (?)
unittests-Solution 79.43% <100.00%> (?)

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

Files Coverage Δ
...on.AWS/Implementation/AWSTracingPipelineHandler.cs 85.41% <100.00%> (-2.34%) ⬇️
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs 94.54% <100.00%> (-0.29%) ⬇️
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs 85.71% <100.00%> (+0.46%) ⬆️
...nTelemetry.Instrumentation.AspNet/AspNetMetrics.cs 100.00% <100.00%> (ø)
.../ElasticsearchRequestPipelineDiagnosticListener.cs 82.20% <100.00%> (ø)
...mplementation/EntityFrameworkDiagnosticListener.cs 52.89% <100.00%> (+0.51%) ⬆️
...trumentation.EventCounters/EventCountersMetrics.cs 81.17% <100.00%> (+1.17%) ⬆️
...Hangfire/Implementation/HangfireInstrumentation.cs 100.00% <100.00%> (ø)
...mplementation/OwinInstrumentationActivitySource.cs 100.00% <100.00%> (ø)
....Owin/Implementation/OwinInstrumentationMetrics.cs 100.00% <100.00%> (ø)
... and 5 more

... and 190 files with indirect coverage changes

@Kielek Kielek marked this pull request as ready for review March 25, 2024 13:11
@Kielek Kielek requested a review from a team March 25, 2024 13:11
@Kielek
Copy link
Contributor Author

Kielek commented Mar 25, 2024

I will be happy to propagate to main repository if we have agree on the naming and values.

@utpilla
Copy link
Contributor

utpilla commented Mar 25, 2024

Why does the ActivitySource version need to match the package version?

@utpilla
Copy link
Contributor

utpilla commented Mar 25, 2024

I will be happy to propagate to main repository if we have agree on the naming and values.

This would probably be considered a breaking change for the stable packages.

@cijothomas
Copy link
Member

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 will be happy to propagate to main repository if we have agree on the naming and values.

This would probably be considered a breaking change for the stable packages.

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.

@vishweshbankwar
Copy link
Member

vishweshbankwar commented Mar 25, 2024

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. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

@utpilla
Copy link
Contributor

utpilla commented Mar 25, 2024

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.

I was thinking about its implications for someone using a custom ActivityListener but I guess we don't need to worry about that since the ActvitySource instances are not marked public for both the stable instrumentation packages.

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 ActivitySource based instrumentation, so we wouldn't have any kind of consistency either.

@cijothomas
Copy link
Member

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.

I was thinking about its implications for someone using a custom ActivityListener but I guess we don't need to worry about that since the ActvitySource instances are not marked public for both the stable instrumentation packages.

Got it!

@cijothomas
Copy link
Member

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. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

Good point. Same challenges for "schema_url" too (not yet supported in .NET)!

@Kielek
Copy link
Contributor Author

Kielek commented Mar 26, 2024

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. OpenTelemetry.Instrumentation.AspNetCore and OpenTelemetry.Instrumentation.HttpClient

So there will still be some inconsistency/gaps.

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 nonzero.0.0.0. Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

@Oberon00
Copy link
Member

For most cases it should be not assembly version, as it typically is nonzero.0.0.0

Shouldn't we fix the assembly version then?

@Kielek
Copy link
Contributor Author

Kielek commented Mar 26, 2024

For most cases it should be not assembly version, as it typically is nonzero.0.0.0

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>());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Suggested change
private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaInstrumentationOptions>());
private static readonly ActivitySource AWSLambdaActivitySource = new(ActivitySourceName, SignalVersionHelper.GetVersion<AWSLambdaWrapper>());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are using !. here, would it be better to use any fallback, or maybe a ?. chain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cijothomas
Copy link
Member

@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?

@utpilla
Copy link
Contributor

utpilla commented Mar 26, 2024

As a follow up to this (and main reposiroty) we should start recommending what to put as a version.

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.

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

How are users using ActivitySource version for debugging purposes?

@Kielek
Copy link
Contributor Author

Kielek commented Mar 26, 2024

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naïve question: Does this have any implication for AOT? Does AOT support getting assembly info?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any compilation warnings. Some instrumentation packages are included into AOT app.

<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWS" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.AWSLambda" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.EventCounters" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.Runtime" />
<TrimmerRootAssembly Include="OpenTelemetry.Instrumentation.StackExchangeRedis" />

@cijothomas
Copy link
Member

Putting such versions is more or less useless if we speaking about telemetry/debugging purposes.

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.

Got it!
Another example: a vendor has some special handling for v1 of semantic conventions, and want to get rid of that special handling, as v2 is out now, and v2 is considered stable. But the vendor need to figure out what percentage of its users is producing v1 vs v2.
This can be understood by looking at instrumentationscope.version.

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.)

@Oberon00
Copy link
Member

Oberon00 commented Mar 28, 2024

@cijothomas

But the vendor need to figure out what percentage of its users is producing v1 vs v2.
This can be understood by looking at instrumentationscope.version.

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.

Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Kielek
Copy link
Contributor Author

Kielek commented Apr 2, 2024

@vishweshbankwar, I created: #5497. Will merge this one shortly (when the CI will finished).

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.