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

[repo] Fix netcoreapp3.1 tests failing in CI #827

Merged
merged 14 commits into from
Dec 15, 2022
Merged
8 changes: 7 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@ jobs:
steps:
- uses: actions/checkout@v3

- uses: actions/setup-dotnet@v3.0.3
- name: Install .NET 3.1 SDK
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/setup-dotnet@v3.0.3
with:
dotnet-version: '3.1.x'

- name: Install .NET 7 SDK
uses: actions/setup-dotnet@v3.0.3
with:
dotnet-version: '7.0.x'

Expand Down
5 changes: 5 additions & 0 deletions .github/workflows/dotnet-format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ jobs:
- name: check out code
uses: actions/checkout@v3

- name: Install .NET 7 SDK
utpilla marked this conversation as resolved.
Show resolved Hide resolved
uses: actions/setup-dotnet@v3.0.3
with:
dotnet-version: '7.0.x'

- name: Install format tool
run: dotnet tool install -g dotnet-format

Expand Down
3 changes: 2 additions & 1 deletion build/Common.nonprod.props
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@
<DotNetXUnitCliVer>[2.3.1,3.0)</DotNetXUnitCliVer>
<MicrosoftExtensionsLoggingPkgVer>[5.0.0,7.0)</MicrosoftExtensionsLoggingPkgVer>
<MicrosoftNETTestSdkPkgVer>[16.11.0,17.0)</MicrosoftNETTestSdkPkgVer>
<MoqPkgVer>[4.17.2,5.0)</MoqPkgVer>
<MoqPkgVer>[4.18.3,5.0)</MoqPkgVer>
<MoqNet45PkgVer>[4.17.2,5.0)</MoqNet45PkgVer>
<OpenTelemetryExporterInMemoryPkgVer>$(OpenTelemetryCoreLatestVersion)</OpenTelemetryExporterInMemoryPkgVer>
<OpenTelemetryExporterInMemoryLatestPreReleasePkgVer>$(OpenTelemetryCoreLatestPrereleaseVersion)</OpenTelemetryExporterInMemoryLatestPreReleasePkgVer>
<XUnitRunnerVisualStudioPkgVer>[2.4.3,3.0)</XUnitRunnerVisualStudioPkgVer>
Expand Down
2 changes: 0 additions & 2 deletions build/Common.props
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
Refer to https://docs.microsoft.com/en-us/nuget/concepts/package-versioning for semver syntax.
-->
<MinVerPkgVer>[4.2.0,5.0)</MinVerPkgVer>
<MicrosoftCodeAnalysisNetAnalyzersPkgVer>[6.0.0]</MicrosoftCodeAnalysisNetAnalyzersPkgVer>
<MicrosoftCodeCoveragePkgVer>[17.3.2]</MicrosoftCodeCoveragePkgVer>
<MicrosoftExtensionsHostingAbstractionsPkgVer>[2.1.0,5.0)</MicrosoftExtensionsHostingAbstractionsPkgVer>
<MicrosoftExtensionsOptionsPkgVer>[3.1.0,)</MicrosoftExtensionsOptionsPkgVer>
Expand All @@ -51,7 +50,6 @@
<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="$(MicrosoftNETFrameworkReferenceAssembliesPkgVer)" PrivateAssets="All" />
<PackageReference Include="StyleCop.Analyzers" Version="$(StyleCopAnalyzersPkgVer)" Condition="'$(SkipAnalysis)'!='true'" PrivateAssets="All" />
<PackageReference Include="Microsoft.CodeCoverage" Version="$(MicrosoftCodeCoveragePkgVer)" Condition="'$(Configuration)'=='Release'" PrivateAssets="All" />
<PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="$(MicrosoftCodeAnalysisNetAnalyzersPkgVer)" Condition="'$(EnableAnalysis)'=='true' and '$(SkipAnalysis)'!='true'" PrivateAssets="All" />
</ItemGroup>

<ItemGroup Condition="'$(IncludeSharedInstrumentationSource)'=='true'">
Expand Down
3 changes: 1 addition & 2 deletions build/Common.targets
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
<Project>

<PropertyGroup Condition="'$(EnableAnalysis)'=='true' and '$(SkipAnalysis)'!='true'">
<AnalysisMode>AllEnabledByDefault</AnalysisMode>
<AnalysisLevel>latest</AnalysisLevel>
<AnalysisLevel>latest-all</AnalysisLevel>
</PropertyGroup>

</Project>
3 changes: 2 additions & 1 deletion build/docker-compose.net6.0.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ services:
build:
args:
PUBLISH_FRAMEWORK: net6.0
SDK_VERSION: 6.0
TEST_SDK_VERSION: 6.0
BUILD_SDK_VERSION: 7.0
3 changes: 2 additions & 1 deletion build/docker-compose.net7.0.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ services:
build:
args:
PUBLISH_FRAMEWORK: net7.0
SDK_VERSION: 7.0
TEST_SDK_VERSION: 7.0
BUILD_SDK_VERSION: 7.0
3 changes: 2 additions & 1 deletion build/docker-compose.netcoreapp3.1.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ services:
build:
args:
PUBLISH_FRAMEWORK: netcoreapp3.1
SDK_VERSION: 3.1
TEST_SDK_VERSION: 3.1
BUILD_SDK_VERSION: 7.0
2 changes: 2 additions & 0 deletions examples/AspNet/Global.asax.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@

namespace Examples.AspNet;

#pragma warning disable SA1649 // File name should match first type name
public class WebApiApplication : HttpApplication
#pragma warning restore SA1649 // File name should match first type name
{
private IDisposable tracerProvider;
private IDisposable meterProvider;
Expand Down
6 changes: 6 additions & 0 deletions global.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"sdk": {
"rollForward": "latestFeature",
"version": "7.0.101"
}
}
4 changes: 3 additions & 1 deletion opentelemetry-dotnet-contrib.sln
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution items", "Solution
ProjectSection(SolutionItems) = preProject
.editorconfig = .editorconfig
CONTRIBUTING.md = CONTRIBUTING.md
global.json = global.json
NuGet.config = NuGet.config
opentelemetry-dotnet-contrib.proj = opentelemetry-dotnet-contrib.proj
examples\process-instrumentation\process-instrumentation.csproj = examples\process-instrumentation\process-instrumentation.csproj
Expand All @@ -26,9 +27,9 @@ EndProject
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "workflows", "workflows", "{43CAFE52-F329-4431-87DA-7FEE1454D9A9}"
ProjectSection(SolutionItems) = preProject
.github\workflows\assign-reviewers.yml = .github\workflows\assign-reviewers.yml
.github\workflows\codeql-analysis.yml = .github\workflows\codeql-analysis.yml
.github\workflows\ci-md.yml = .github\workflows\ci-md.yml
.github\workflows\ci.yml = .github\workflows\ci.yml
.github\workflows\codeql-analysis.yml = .github\workflows\codeql-analysis.yml
.github\workflows\dotnet-core-cov.yml = .github\workflows\dotnet-core-cov.yml
.github\workflows\dotnet-format-md.yml = .github\workflows\dotnet-format-md.yml
.github\workflows\dotnet-format.yml = .github\workflows\dotnet-format.yml
Expand Down Expand Up @@ -73,6 +74,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "build", "build", "{824BD1DE
build\Common.targets = build\Common.targets
build\debug.snk = build\debug.snk
build\docker-compose.net6.0.yml = build\docker-compose.net6.0.yml
build\docker-compose.net7.0.yml = build\docker-compose.net7.0.yml
build\docker-compose.netcoreapp3.1.yml = build\docker-compose.netcoreapp3.1.yml
build\opentelemetry-icon-color.png = build\opentelemetry-icon-color.png
build\OpenTelemetryContrib.prod.ruleset = build\OpenTelemetryContrib.prod.ruleset
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ public ConnectionStringBuilder(string connectionString)
continue;
}

#if NET6_0_OR_GREATER
var index = token.IndexOf(EqualSign, StringComparison.Ordinal);
#else
var index = token.IndexOf(EqualSign);
#endif
if (index == -1 || index != token.LastIndexOf(EqualSign))
{
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ internal static class MessagePackSerializer
private const int LIMIT_MAX_FIX_ARRAY_LENGTH = 15;
private const int STRING_SIZE_LIMIT_CHAR_COUNT = (1 << 14) - 1; // 16 * 1024 - 1 = 16383

#if NET6_0_OR_GREATER
private const int MAX_STACK_ALLOC_SIZE_IN_BYTES = 256;
#endif

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int SerializeNull(byte[] buffer, int cursor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public override void OnEnd(LogRecord data)
}
}

private class State
private sealed class State
{
public State(ActivityTagsCollection tags, ActivityEventAttachingLogProcessor processor)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace OpenTelemetry;
/// EventSource implementation for OpenTelemetry SDK extensions implementation.
/// </summary>
[EventSource(Name = "OpenTelemetry-Extensions")]
internal class OpenTelemetryExtensionsEventSource : EventSource
internal sealed class OpenTelemetryExtensionsEventSource : EventSource
{
public static OpenTelemetryExtensionsEventSource Log = new OpenTelemetryExtensionsEventSource();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ public static MeterProviderBuilder AddProcessInstrumentation(
var options = new ProcessInstrumentationOptions();
configure?.Invoke(options);

var instrumentation = new ProcessMetrics(options);
builder.AddMeter(instrumentation.MeterInstance.Name);
return builder.AddInstrumentation(() => instrumentation);
builder.AddMeter(ProcessMetrics.MeterName);

return builder.AddInstrumentation(() => new ProcessMetrics(options));
}
}
21 changes: 14 additions & 7 deletions src/OpenTelemetry.Instrumentation.Process/ProcessMetrics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@

namespace OpenTelemetry.Instrumentation.Process;

internal sealed class ProcessMetrics
internal sealed class ProcessMetrics : IDisposable
{
internal static readonly AssemblyName AssemblyName = typeof(ProcessMetrics).Assembly.GetName();
internal readonly Meter MeterInstance = new(AssemblyName.Name, AssemblyName.Version.ToString());
internal static readonly string MeterName = AssemblyName.Name;

private readonly Meter meterInstance = new(MeterName, AssemblyName.Version.ToString());

// vars for calculating CPU utilization
private DateTime lastCollectionTimeUtc;
Expand All @@ -38,7 +40,7 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
this.lastCollectedUserProcessorTime = Diagnostics.Process.GetCurrentProcess().UserProcessorTime.TotalSeconds;
this.lastCollectedPrivilegedProcessorTime = Diagnostics.Process.GetCurrentProcess().PrivilegedProcessorTime.TotalSeconds;

this.MeterInstance.CreateObservableUpDownCounter(
this.meterInstance.CreateObservableUpDownCounter(
"process.memory.usage",
() =>
{
Expand All @@ -47,7 +49,7 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
unit: "By",
description: "The amount of physical memory allocated for this process.");

this.MeterInstance.CreateObservableUpDownCounter(
this.meterInstance.CreateObservableUpDownCounter(
"process.memory.virtual",
() =>
{
Expand All @@ -56,7 +58,7 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
unit: "By",
description: "The amount of committed virtual memory for this process.");

this.MeterInstance.CreateObservableCounter(
this.meterInstance.CreateObservableCounter(
"process.cpu.time",
() =>
{
Expand All @@ -70,7 +72,7 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
unit: "s",
description: "Total CPU seconds broken down by different states.");

this.MeterInstance.CreateObservableGauge(
this.meterInstance.CreateObservableGauge(
"process.cpu.utilization",
() =>
{
Expand All @@ -79,7 +81,7 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
unit: "1",
description: "Difference in process.cpu.time since the last measurement, divided by the elapsed time and number of CPUs available to the process.");

this.MeterInstance.CreateObservableUpDownCounter(
this.meterInstance.CreateObservableUpDownCounter(
"process.threads",
() =>
{
Expand All @@ -89,6 +91,11 @@ public ProcessMetrics(ProcessInstrumentationOptions options)
description: "Process threads count.");
}

public void Dispose()
{
this.meterInstance.Dispose();
Copy link
Contributor

@utpilla utpilla Dec 15, 2022

Choose a reason for hiding this comment

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

I'm not sure if it's safe to do this in a scenario where you have multiple Sdks each with an instance of ProcessMetrics added since they would be using the same Meter name (even though each instance of ProcessMetrics would create a new instance of Meter).

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test and yes, it does appear to be an issue 😭 Going to disable the test for now. I think multiple provider case is rare so it isn't super urgent that we fix this. But I'm going to spin up an issue and work with @Yun-Ting on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #831

}

private IEnumerable<Measurement<double>> GetCpuUtilization()
{
var process = Diagnostics.Process.GetCurrentProcess();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Unit test project for AWS X-Ray for OpenTelemetry</Description>
Expand All @@ -9,7 +9,7 @@
<ItemGroup>
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(MicrosoftNETTestSdkPkgVer)" />
<PackageReference Include="xunit" Version="$(XUnitPkgVer)" />
<PackageReference Include="Moq" Version="$(MoqPkgVer)" />
<PackageReference Include="Moq" Version="$(MoqNet45PkgVer)" />
<PackageReference Condition="$([MSBuild]::IsOsPlatform('Windows'))" Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioPkgVer)">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Unit test project for AWS client instrumentation for OpenTelemetry</Description>
Expand All @@ -12,7 +12,7 @@
<PackageReference Include="AWSSDK.Core" Version="3.5.1.24" />
<PackageReference Include="AWSSDK.SQS" Version="3.5.0.26" />
<PackageReference Include="AWSSDK.DynamoDBv2" Version="3.5.1.2" />
<PackageReference Include="Moq" Version="$(MoqPkgVer)" />
<PackageReference Include="Moq" Version="$(MoqNet45PkgVer)" />
<PackageReference Condition="$([MSBuild]::IsOsPlatform('Windows'))" Include="xunit.runner.visualstudio" Version="$(XUnitRunnerVisualStudioPkgVer)">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public void AutoFlushActivityProcessor_FlushAfterLocalServerSideRootSpans_EndMat

using var source = new ActivitySource(sourceName);
using var activity = source.StartActivity("name", ActivityKind.Server);
activity.Dispose();
activity.Stop();
Copy link
Member Author

Choose a reason for hiding this comment

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

@utpilla FYI this change didn't help, but bumping the version of Moq actually seems to have finally stabilized this test. I looked through the CHANGELOG but nothing jumped out at me as an obvious fix 🤷

Relates to #814


mockExporting.Protected().Verify("OnForceFlush", Times.Once(), 5_000);
}
Expand All @@ -59,7 +59,7 @@ public void AutoFlushActivityProcessor_FlushAfterLocalServerSideRootSpans_EndNon

using var source = new ActivitySource(sourceName);
using var activity = source.StartActivity("name", ActivityKind.Client);
activity.Dispose();
activity.Stop();

mockExporting.Protected().Verify("OnForceFlush", Times.Never(), It.IsAny<int>());
}
Expand All @@ -78,7 +78,7 @@ public void AutoFlushActivityProcessor_PredicateThrows_DoesNothing()

using var source = new ActivitySource(sourceName);
using var activity = source.StartActivity("name", ActivityKind.Server);
activity.Dispose();
activity.Stop();

mockExporting.Protected().Verify("OnForceFlush", Times.Never(), 5_000);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

<PropertyGroup>
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
<TargetFrameworks>net7.0;net6.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks>net7.0;net6.0</TargetFrameworks>
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net462</TargetFrameworks>
</PropertyGroup>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ public void CpuUtilizationMetricsAreCaptured()
Assert.True(systemCpuUtilizationCaptured);
}

[Fact]
// See: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/831
[Fact(Skip = "There are known issues with this test.")]
public void CheckValidGaugeValueWhen2MeterProviderInstancesHaveTheSameMeterName()
{
var exportedItemsA = new List<Metric>();
Expand All @@ -134,19 +135,45 @@ public void CheckValidGaugeValueWhen2MeterProviderInstancesHaveTheSameMeterName(
.AddInMemoryExporter(exportedItemsA)
.Build();

using var meterProviderB = Sdk.CreateMeterProviderBuilder()
using (var meterProviderB = Sdk.CreateMeterProviderBuilder()
.AddProcessInstrumentation()
.AddInMemoryExporter(exportedItemsB)
.Build();
.Build())
{
meterProviderA.ForceFlush(MaxTimeToAllowForFlush);
meterProviderB.ForceFlush(MaxTimeToAllowForFlush);

var metricA = exportedItemsA.FirstOrDefault(i => i.Name == "process.memory.usage");
var metricB = exportedItemsB.FirstOrDefault(i => i.Name == "process.memory.usage");

Assert.True(GetValue(metricA) > 0);
Assert.True(GetValue(metricB) > 0);
}

exportedItemsA.Clear();
exportedItemsB.Clear();

meterProviderA.ForceFlush(MaxTimeToAllowForFlush);
meterProviderB.ForceFlush(MaxTimeToAllowForFlush);

var metricA = exportedItemsA.FirstOrDefault(i => i.Name == "process.memory.usage");
var metricB = exportedItemsB.FirstOrDefault(i => i.Name == "process.memory.usage");
Assert.NotEmpty(exportedItemsA);
Assert.Empty(exportedItemsB);

exportedItemsA.Clear();
exportedItemsB.Clear();

meterProviderA.ForceFlush(MaxTimeToAllowForFlush);

Assert.NotEmpty(exportedItemsA);
Assert.Empty(exportedItemsB);

exportedItemsA.Clear();

meterProviderA.ForceFlush(MaxTimeToAllowForFlush);

Assert.True(GetValue(metricA) > 0);
Assert.True(GetValue(metricB) > 0);
// Note: This fails due to:
// https://github.com/open-telemetry/opentelemetry-dotnet/blob/main/src/OpenTelemetry/Metrics/MetricReaderExt.cs#L244-L249
Assert.NotEmpty(exportedItemsA);
Assert.Empty(exportedItemsB);
}

private static double GetValue(Metric metric)
Expand Down
Loading