Replace InstrumentRecorder with MetricCollector #48931
Conversation
JamesNK
force-pushed
the
jamesnk/metric-collector
branch
from
7d429b0
to
53c393c
Compare
JamesNK
requested review from
BrennanConroy,
halter73,
Tratcher,
mgravell,
javiercn,
a team and
wtgodbe
as code owners
|
Wow, the build passed. @wtgodbe Please double-check the necessary add-dependency rituals have been performed. |
| @@ -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> | |||
There was a problem hiding this comment.
interesting, @geeknoid changes already published to NuGet?
eng/Version.Details.xml
Outdated
| @@ -41,6 +41,10 @@ | |||
| <Uri>https:/dotnet/efcore</Uri> | |||
| <Sha>b0b202671f7879070423e95776edc014885335ad</Sha> | |||
| </Dependency> | |||
| <Dependency Name="Microsoft.Extensions.Telemetry.Testing" Version="8.0.0-preview.6.23320.3"> | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Or C: Do not allow circular dependencies.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
I've just woken up and have meetings for the next couple of hours 😬
There was a problem hiding this comment.
To confirm, this dependency needs to be moved to Toolset before this gets checked in.
There was a problem hiding this comment.
Subscription created
https:/dotnet/extensions (.NET 8) ==> 'https:/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
There was a problem hiding this comment.
To confirm, this dependency needs to be moved to Toolset before this gets checked in.
Moved dependency to toolset.
ghost
added this to the 
amcasey
added
area-networking
InstrumentRecorder is being replaced by MetricCollector.
The notable change is the PR references a dotnet/extensions package for testing. I don't know if we've done that before in dotnet/aspnetcore. @wtgodbe I'll need your help getting the new dependency setup correctly.