Skip to content

Replace InstrumentRecorder with MetricCollector #48931

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 22, 2023
Merged

Replace InstrumentRecorder with MetricCollector #48931

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
da37b24
Replace InstrumentRecorder with MetricCollector
JamesNK Jun 21, 2023
53c393c
Everywhere
JamesNK Jun 21, 2023
e3e0969
Fix build?
JamesNK Jun 21, 2023
634292e
Move to ToolsetDependencies
JamesNK Jun 22, 2023
97c66b0
Move to ToolsetDependencies
JamesNK Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eng/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ and are generated based on the last package release.
<LatestPackageReference Include="Microsoft.Extensions.Options.DataAnnotations" />
<LatestPackageReference Include="Microsoft.Extensions.Options" />
<LatestPackageReference Include="Microsoft.Extensions.Primitives" />
<LatestPackageReference Include="Microsoft.Extensions.Telemetry.Testing" />
<LatestPackageReference Include="Microsoft.Win32.Registry" />
<LatestPackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" />
<LatestPackageReference Include="Microsoft.CodeAnalysis.CSharp.CodeFix.Testing.XUnit" />
Expand Down
4 changes: 4 additions & 0 deletions eng/Version.Details.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@
<Uri>https://github.com/dotnet/efcore</Uri>
<Sha>b0b202671f7879070423e95776edc014885335ad</Sha>
</Dependency>
<Dependency Name="Microsoft.Extensions.Telemetry.Testing" Version="8.0.0-preview.6.23320.3">
Copy link
Member

Choose a reason for hiding this comment

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

If this is from the main branch, then everything's set up correctly in this PR. All we'll need is to add a subscription from extensions into this repo. The only issue there is it creates a circular dependency between this repo and extensions, which can be a problem for product construction. I think it'll be fine if we set up the extensions -> aspnetcore sub to be everyWeek frequency - @mmitche to confirm

Copy link
Member

Choose a reason for hiding this comment

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

You beat me to it :P, I was about to ask about the circular dependency for this. Keep in mind this dependency is for use in tests only, so I think that in the case that darc doesn't allow to setup this subscription could go with two options:

  • Either add support in dependency flow for test-only dependencies (similar to how we have special case for tooling today) so we can introduce ciruclar dependencies on these cases, or
  • Not use dependency flow for test-only dependencies that could introduce ciruclar dependencies for repos. If we go with that plan, then we undo the change to this Version.Details.xml file and we just update the version we depend on in a as-needed basis.

Copy link
Member

Choose a reason for hiding this comment

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

Or C: Do not allow circular dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Circular dependencies are allowed for testing purposes. However, this must be moved to the ToolsetDependencies section otherwise the product is incoherent and cannot be shipped. What this means is:

  • dotnet/extensions outputs will not show in the build drop.
  • dotnet/extensions outputs is not part of the main SDK-umbrella product.

It's my understanding that this is the intention. If it's not, then we have a product layering issue, or we need to change how we prep the product for shipping. As usual, that would make me question why we have a separate repo for these things in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, the intention is for this to be test-only and shouldn't be in the build drop nor part of the main SDK product. @mmitche so is the concrete feedback then that we should move these 4 lines on this file down to <ToolsetDependencies> section (line 360), and then that should allow us to setup darc subscription correctly? Or is there something else that needs to be done to setup the darc subscription?

Copy link
Member

Choose a reason for hiding this comment

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

@JamesNK @joperezr let me know if you need my help setting up the subscription

Copy link
Member Author

@JamesNK JamesNK Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes please.

Note: The InstrumentRecorder has just been removed from runtime (for this preview) so this PR needs to make it in, otherwise we'll be blocked when consuming runtime updates.

dotnet/runtime#87873

I've just woken up and have meetings for the next couple of hours 😬

Copy link
Member

@mmitche mmitche Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To confirm, this dependency needs to be moved to Toolset before this gets checked in.

Copy link
Member

Choose a reason for hiding this comment

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

Subscription created

https://github.com/dotnet/extensions (.NET 8) ==> 'https://github.com/dotnet/aspnetcore' ('main')
  - Id: ba55bbf2-4239-468a-a9c6-08db6772cb2c
  - Update Frequency: EveryWeek
  - Enabled: True
  - Batchable: False
  - PR Failure Notification tags: 
  - Merge Policies:
    AllChecksSuccessful
      ignoreChecks = 
                       [
                         "aspnetcore-quarantined-pr",
                         "aspnetcore-quarantined-pr (Tests: Helix),",
                         "aspnetcore-quarantined-pr (Tests: macOS),",
                         "aspnetcore-quarantined-pr (Tests: Ubuntu x64),",
                         "aspnetcore-quarantined-pr (Tests: Windows x64)"
                       ]
    NoRequestedChanges

Copy link
Member Author

Choose a reason for hiding this comment

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

To confirm, this dependency needs to be moved to Toolset before this gets checked in.

Moved dependency to toolset.

<Uri>https://github.com/dotnet/extensions</Uri>
<Sha>a0e9c8794e3e0ba27033a9f54a545385228d0876</Sha>
</Dependency>
<Dependency Name="Microsoft.Extensions.Caching.Abstractions" Version="8.0.0-preview.6.23318.9">
<Uri>https://github.com/dotnet/runtime</Uri>
<Sha>815953a12c822847095a843d69c610a9f895ae3f</Sha>
Expand Down
2 changes: 2 additions & 0 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@
<SystemDiagnosticsPerformanceCounterVersion>8.0.0-preview.6.23318.9</SystemDiagnosticsPerformanceCounterVersion>
<SystemIOHashingVersion>8.0.0-preview.6.23318.9</SystemIOHashingVersion>
<SystemRuntimeCachingVersion>8.0.0-preview.6.23318.9</SystemRuntimeCachingVersion>
<!-- Packages from dotnet/extensions -->
<MicrosoftExtensionsTelemetryTestingVersion>8.0.0-preview.6.23320.3</MicrosoftExtensionsTelemetryTestingVersion>
Copy link
Member

Choose a reason for hiding this comment

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

8.0.0-preview.6.23320.3<

interesting, @geeknoid changes already published to NuGet?

<!-- Packages from dotnet/efcore -->
<dotnetefVersion>8.0.0-preview.6.23319.5</dotnetefVersion>
<MicrosoftEntityFrameworkCoreInMemoryVersion>8.0.0-preview.6.23319.5</MicrosoftEntityFrameworkCoreInMemoryVersion>
Expand Down
26 changes: 13 additions & 13 deletions src/Hosting/Hosting/test/HostingApplicationDiagnosticsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.Metrics;
using System.Diagnostics.Tracing;
using System.Reflection;
using Microsoft.AspNetCore.Http;
Expand All @@ -12,6 +11,7 @@
using Microsoft.Extensions.Diagnostics.Metrics;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Testing;
using Microsoft.Extensions.Telemetry.Testing.Metering;
using Moq;

namespace Microsoft.AspNetCore.Hosting.Tests;
Expand Down Expand Up @@ -52,10 +52,10 @@ public async Task EventCountersAndMetricsValues()
var hostingApplication1 = CreateApplication(out var features1, eventSource: hostingEventSource, meterFactory: testMeterFactory1);
var hostingApplication2 = CreateApplication(out var features2, eventSource: hostingEventSource, meterFactory: testMeterFactory2);

using var currentRequestsRecorder1 = new InstrumentRecorder<long>(testMeterFactory1, HostingMetrics.MeterName, "http-server-current-requests");
using var currentRequestsRecorder2 = new InstrumentRecorder<long>(testMeterFactory2, HostingMetrics.MeterName, "http-server-current-requests");
using var requestDurationRecorder1 = new InstrumentRecorder<double>(testMeterFactory1, HostingMetrics.MeterName, "http-server-request-duration");
using var requestDurationRecorder2 = new InstrumentRecorder<double>(testMeterFactory2, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsRecorder1 = new MetricCollector<long>(testMeterFactory1, HostingMetrics.MeterName, "http-server-current-requests");
using var currentRequestsRecorder2 = new MetricCollector<long>(testMeterFactory2, HostingMetrics.MeterName, "http-server-current-requests");
using var requestDurationRecorder1 = new MetricCollector<double>(testMeterFactory1, HostingMetrics.MeterName, "http-server-request-duration");
using var requestDurationRecorder2 = new MetricCollector<double>(testMeterFactory2, HostingMetrics.MeterName, "http-server-request-duration");

// Act/Assert 1
var context1 = hostingApplication1.CreateContext(features1);
Expand All @@ -74,15 +74,15 @@ public async Task EventCountersAndMetricsValues()
Assert.Equal(0, await currentRequestValues.FirstOrDefault(v => v == 0));
Assert.Equal(0, await failedRequestValues.FirstOrDefault(v => v == 0));

Assert.Collection(currentRequestsRecorder1.GetMeasurements(),
Assert.Collection(currentRequestsRecorder1.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(currentRequestsRecorder2.GetMeasurements(),
Assert.Collection(currentRequestsRecorder2.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder1.GetMeasurements(),
Assert.Collection(requestDurationRecorder1.GetMeasurementSnapshot(),
m => Assert.True(m.Value > 0));
Assert.Collection(requestDurationRecorder2.GetMeasurements(),
Assert.Collection(requestDurationRecorder2.GetMeasurementSnapshot(),
m => Assert.True(m.Value > 0));

// Act/Assert 2
Expand All @@ -105,20 +105,20 @@ public async Task EventCountersAndMetricsValues()
Assert.Equal(0, await currentRequestValues.FirstOrDefault(v => v == 0));
Assert.Equal(2, await failedRequestValues.FirstOrDefault(v => v == 2));

Assert.Collection(currentRequestsRecorder1.GetMeasurements(),
Assert.Collection(currentRequestsRecorder1.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(currentRequestsRecorder2.GetMeasurements(),
Assert.Collection(currentRequestsRecorder2.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder1.GetMeasurements(),
Assert.Collection(requestDurationRecorder1.GetMeasurementSnapshot(),
m => Assert.True(m.Value > 0),
m => Assert.True(m.Value > 0));
Assert.Collection(requestDurationRecorder2.GetMeasurements(),
Assert.Collection(requestDurationRecorder2.GetMeasurementSnapshot(),
m => Assert.True(m.Value > 0),
m => Assert.True(m.Value > 0));
}
Expand Down
43 changes: 22 additions & 21 deletions src/Hosting/Hosting/test/HostingMetricsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Diagnostics.Metrics;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Telemetry.Testing.Metering;

namespace Microsoft.AspNetCore.Hosting.Tests;

Expand All @@ -25,9 +26,9 @@ public void MultipleRequests()
var httpContext = new DefaultHttpContext();
var meter = meterFactory.Meters.Single();

using var requestDurationRecorder = new InstrumentRecorder<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsRecorder = new InstrumentRecorder<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");
using var unhandledRequestsRecorder = new InstrumentRecorder<long>(meterFactory, HostingMetrics.MeterName, "http-server-unhandled-requests");
using var requestDurationCollector = new MetricCollector<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsCollector = new MetricCollector<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");
using var unhandledRequestsCollector = new MetricCollector<long>(meterFactory, HostingMetrics.MeterName, "http-server-unhandled-requests");

// Act/Assert
Assert.Equal(HostingMetrics.MeterName, meter.Name);
Expand All @@ -39,10 +40,10 @@ public void MultipleRequests()
context1.HttpContext.Response.StatusCode = StatusCodes.Status200OK;
hostingApplication.DisposeContext(context1, null);

Assert.Collection(currentRequestsRecorder.GetMeasurements(),
Assert.Collection(currentRequestsCollector.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder.GetMeasurements(),
Assert.Collection(requestDurationCollector.GetMeasurementSnapshot(),
m => AssertRequestDuration(m, HttpProtocol.Http11, StatusCodes.Status200OK));

// Request 2 (after failure)
Expand All @@ -51,12 +52,12 @@ public void MultipleRequests()
context2.HttpContext.Response.StatusCode = StatusCodes.Status500InternalServerError;
hostingApplication.DisposeContext(context2, new InvalidOperationException("Test error"));

Assert.Collection(currentRequestsRecorder.GetMeasurements(),
Assert.Collection(currentRequestsCollector.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder.GetMeasurements(),
Assert.Collection(requestDurationCollector.GetMeasurementSnapshot(),
m => AssertRequestDuration(m, HttpProtocol.Http11, StatusCodes.Status200OK),
m => AssertRequestDuration(m, HttpProtocol.Http2, StatusCodes.Status500InternalServerError, exceptionName: "System.InvalidOperationException"));

Expand All @@ -66,37 +67,37 @@ public void MultipleRequests()
context3.HttpContext.Items["__RequestUnhandled"] = true;
context3.HttpContext.Response.StatusCode = StatusCodes.Status404NotFound;

Assert.Collection(currentRequestsRecorder.GetMeasurements(),
Assert.Collection(currentRequestsCollector.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value));
Assert.Collection(requestDurationRecorder.GetMeasurements(),
Assert.Collection(requestDurationCollector.GetMeasurementSnapshot(),
m => AssertRequestDuration(m, HttpProtocol.Http11, StatusCodes.Status200OK),
m => AssertRequestDuration(m, HttpProtocol.Http2, StatusCodes.Status500InternalServerError, exceptionName: "System.InvalidOperationException"));

hostingApplication.DisposeContext(context3, null);

Assert.Collection(currentRequestsRecorder.GetMeasurements(),
Assert.Collection(currentRequestsCollector.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value),
m => Assert.Equal(1, m.Value),
m => Assert.Equal(-1, m.Value));
Assert.Collection(requestDurationRecorder.GetMeasurements(),
Assert.Collection(requestDurationCollector.GetMeasurementSnapshot(),
m => AssertRequestDuration(m, HttpProtocol.Http11, StatusCodes.Status200OK),
m => AssertRequestDuration(m, HttpProtocol.Http2, StatusCodes.Status500InternalServerError, exceptionName: "System.InvalidOperationException"),
m => AssertRequestDuration(m, HttpProtocol.Http3, StatusCodes.Status404NotFound));
Assert.Collection(unhandledRequestsRecorder.GetMeasurements(),
Assert.Collection(unhandledRequestsCollector.GetMeasurementSnapshot(),
m => Assert.Equal(1, m.Value));

static void AssertRequestDuration(Measurement<double> measurement, string protocol, int statusCode, string exceptionName = null)
static void AssertRequestDuration(CollectedMeasurement<double> measurement, string protocol, int statusCode, string exceptionName = null)
{
Assert.True(measurement.Value > 0);
Assert.Equal(protocol, (string)measurement.Tags.ToArray().Single(t => t.Key == "protocol").Value);
Assert.Equal(statusCode, (int)measurement.Tags.ToArray().Single(t => t.Key == "status-code").Value);
Assert.Equal(protocol, (string)measurement.Tags["protocol"]);
Assert.Equal(statusCode, (int)measurement.Tags["status-code"]);
if (exceptionName == null)
{
Assert.DoesNotContain(measurement.Tags.ToArray(), t => t.Key == "exception-name");
Expand Down Expand Up @@ -132,17 +133,17 @@ public async Task StartListeningDuringRequest_NotMeasured()

await syncPoint.WaitForSyncPoint().DefaultTimeout();

using var requestDurationRecorder = new InstrumentRecorder<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsRecorder = new InstrumentRecorder<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");
using var requestDurationCollector = new MetricCollector<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsCollector = new MetricCollector<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");
context1.HttpContext.Response.StatusCode = StatusCodes.Status200OK;

syncPoint.Continue();
await processRequestTask.DefaultTimeout();

hostingApplication.DisposeContext(context1, null);

Assert.Empty(currentRequestsRecorder.GetMeasurements());
Assert.Empty(requestDurationRecorder.GetMeasurements());
Assert.Empty(currentRequestsCollector.GetMeasurementSnapshot());
Assert.Empty(requestDurationCollector.GetMeasurementSnapshot());
}

[Fact]
Expand All @@ -154,8 +155,8 @@ public void IHttpMetricsTagsFeatureNotUsedFromFeatureCollection()
var httpContext = new DefaultHttpContext();
var meter = meterFactory.Meters.Single();

using var requestDurationRecorder = new InstrumentRecorder<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsRecorder = new InstrumentRecorder<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");
using var requestDurationCollector = new MetricCollector<double>(meterFactory, HostingMetrics.MeterName, "http-server-request-duration");
using var currentRequestsCollector = new MetricCollector<long>(meterFactory, HostingMetrics.MeterName, "http-server-current-requests");

// Act/Assert
Assert.Equal(HostingMetrics.MeterName, meter.Name);
Expand Down
1 change: 1 addition & 0 deletions src/Hosting/Hosting/test/Microsoft.AspNetCore.Hosting.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
<Reference Include="Microsoft.AspNetCore.Hosting" />
<Reference Include="Microsoft.Extensions.Hosting" />
<Reference Include="Microsoft.Extensions.Options" />
<Reference Include="Microsoft.Extensions.Telemetry.Testing" />
<Reference Include="System.Threading.Channels" />
</ItemGroup>

Expand Down
1 change: 1 addition & 0 deletions src/Http/Routing/test/UnitTests/Microsoft.AspNetCore.Routing.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
<Reference Include="Microsoft.Extensions.Diagnostics" />
<Reference Include="Microsoft.Extensions.Hosting.Abstractions" />
<Reference Include="Microsoft.Extensions.Logging" />
<Reference Include="Microsoft.Extensions.Telemetry.Testing" />
<Reference Include="Microsoft.Extensions.WebEncoders" />

<Compile Include="$(SharedSourceRoot)Metrics\TestMeterFactory.cs" LinkBase="shared" />
Expand Down
Loading