Repository navigation
Scope pinned metric instruments to resources - #21711
David Pine (IEvangelist) wants to merge 10 commits into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 21711Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 21711" |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Replica-group pinning can produce no cards, persistence conflates null and empty instance IDs and loses pins after reload, and pooled test connections can prevent temporary database cleanup on Windows.
3 open issues
What changed in this PR
This PR makes dashboard metric pins resource-specific, addressing #21710 so matching instruments from different resources or replicas can be pinned independently. Existing global pins retain their previous behavior during migration.
Changes:
- Adds resource identity to pin storage and migrates legacy records.
- Passes resource keys through card and chart controls and pinned-group counts.
- Updates tests for scoped persistence, independent pinning, and legacy migration.
| File | Description |
|---|---|
| tests/Aspire.Dashboard.Tests/Model/MetricPinStoreTests.cs | Tests resource isolation, persistence, and legacy migration. |
| tests/Aspire.Dashboard.Tests/Model/DashboardSettingsDatabaseTests.cs | Updates fallback assertions for resource-scoped pins. |
| tests/Aspire.Dashboard.Components.Tests/Pages/MetricsTests.cs | Adds independent card-pinning coverage and updates scoped assertions. |
| src/Aspire.Dashboard/Model/MetricPinStore.cs | Stores resource-scoped pins and migrates legacy global records. |
| src/Aspire.Dashboard/Components/Controls/MetricGroupList.razor.cs | Counts pins using resource identity. |
| src/Aspire.Dashboard/Components/Controls/Chart/MetricPinButton.razor | Requires a resource key for pin operations. |
| src/Aspire.Dashboard/Components/Controls/Chart/MeterOverview.razor | Supplies each card’s emitting resource key. |
| src/Aspire.Dashboard/Components/Controls/Chart/ChartContainer.razor | Supplies a resource key to the chart pin control. |
Review effort: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Pin filtering and counts use deduplicated instrument summaries, so pinning only a non-retained replica can leave the replica-group pinned view empty; preserve per-replica summaries and add regression coverage.
1 open issue
3 resolved since last review
Review effort: Balanced
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
In replica-group views, pinned cards show group-aggregated metrics, and opening a non-first replica’s card targets the first replica’s chart and pin state instead.
2 open issues
1 resolved since last review
Review effort: Balanced
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Replica-group pinned cards lack identifying replica labels, and “Back to Pinned” restores only the emitting replica’s pins instead of the original group’s pins.
2 open issues
2 resolved since last review
Review effort: Balanced
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed to evaluate subtle navigation-state and replica-group pinning issues involving layout selection, null instance IDs, and resource-name casing.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
Sébastien Ros (sebastienros)
left a comment
Sébastien Ros (sebastienros)
left a comment
There was a problem hiding this comment.
The persistence problem below should be fixed before merge: pins scoped by instance ID are lost on each AppHost restart. The other comments are smaller and could follow up separately.
- Correctness: 3 (pins lost across runs, Back in
ResourcesLayout, aggregated cards pinning one replica) - Minor robustness: 2 (name comparison, schema creation race)
I found these by reading the code at 4dbd4aa and did not run them. CI is green for this commit, but its tests don't cover restarting with new instance IDs or running under the layout.
This comment has been minimized.
This comment has been minimized.
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
9bd111d to
748e22f
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Active issues cause searches started from Pinned to pin only a replica instead of its group and indexed scoped-schema migrations to fail, leaving saved pins unloaded and persistence disabled.
2 open issues
Review effort: Balanced
Keep replica-group pin identity across filtered and chart views, stabilize telemetry-only pins, simplify schema migration, and clarify group unpin behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🟡 Changes recommended
Telemetry-only resource pins can move to another replica when an earlier-sorting instance joins because identity is derived from list position rather than an instance-specific fallback.
1 open issue
2 resolved since last review
1 file not reviewed
src/Aspire.Dashboard/Resources/Metrics.Designer.cs: Generated file
Review effort: Balanced
Keep pins attached to the emitting instance when earlier-sorting telemetry resources arrive, while retaining app-model replica identities. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector14 / 101 PR test projects · 2 PR jobs, from 30 changed files. Selected PR test projects (14 / 101)
Selected PR jobs (2)
How these were chosen — grouped by what changed
🔧 📦 affected project 🔧 🧪 🧪 🧪 Job reasons
Selection computed for commit |
There was a problem hiding this comment.
🔵 Needs a closer look
Human assessment is needed because timing-dependent interactions between replica discovery and instrument refresh can leave saved metric pins temporarily missing.
No open issues 🎉
1 resolved since last review
1 file not reviewed
src/Aspire.Dashboard/Resources/Metrics.Designer.cs: Generated file
Review effort: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Sébastien Ros (sebastienros)
left a comment
Sébastien Ros (sebastienros)
left a comment
There was a problem hiding this comment.
This needs a rework before it can merge. The PR now conflicts with main: #21840 moved metric pins from the settings database to browser local storage (LocalStorageSetting<List<OtlpInstrumentKey>>) and deleted DashboardSettingsDatabaseTests.cs. The SQLite schema, migration and tests in MetricPinStore can't be carried over as they are.
The resource-scoped pins will need a new local-storage shape. Values already stored as a plain list of instrument keys should still read as global pins, which is what the SQLite migration did for the old table. My earlier approval was for the SQLite version, so the persistence code will need another look once it's rebuilt.
I checked the changes since c0823e492b:
- Fixed: filter results opened from the Pinned view, the aggregate chart, the
Resolvelookup cost and the development-schema migration. - Kept as a tradeoff: telemetry-only resources still save their instance ID, which the new code comment explains. The reply on that thread still describes the earlier ordinal fallback, which was later reverted.
- Not fixed: the label on replica cards for group pins (inline below).
This comes from reading the diff at b2de2a4102 and a local merge check against main. I didn't run the dashboard.
|
|
||
| @{ | ||
| var label = _isPinned ? Loc[nameof(MetricsResources.MetricsUnpinInstrument)] : Loc[nameof(MetricsResources.MetricsPinInstrument)]; | ||
| var label = ResourceKey.IsGroup |
There was a problem hiding this comment.
Replica cards still don't say that Unpin removes the pin for all replicas. This label depends on the button's own key, but the case from the earlier thread uses replica keys. In a replica group's Pinned view, OverviewGroupPinResourceKey is null, so each card's button gets (api, 1) or (api, 2), and IsGroup is false. The tooltip says "Unpin metric", yet SetPinned finds the group pin through Matches and deletes it, so the other replica's card disappears too.
The new test checks the tooltip only on a group card, where the button already has the group key. The label needs to come from the pin that matched, for example through a MetricPinStore method that reports whether the matching pin is a group pin.
| public MetricPinResourceKey? GroupPinResourceKey { get; set; } | ||
|
|
||
| [Parameter] | ||
| public IReadOnlyList<OtlpResource>? Resources { get; set; } |
There was a problem hiding this comment.
Nothing reads this parameter: Resolve no longer takes a telemetry resource list, and neither MeterOverview nor LiveSparklinesBase uses Resources. Removing it, along with the Resources="@_resources" in Metrics.razor and the two builder.Add(component => component.Resources, ...) calls in MetricsTests, avoids suggesting that cards depend on it.
Description
Pinning a metric instrument from one resource previously pinned every resource that emitted the same meter and instrument. This made the pinned section broader than the user's action, especially for
Aspire.TerminalHostmetrics emitted by several terminal resources.This change scopes new metric pins to the emitting telemetry resource key. Resources and replicas can now pin or unpin the same instrument independently in both all-resource and resource-scoped Metrics views. The card and chart pin controls pass the actual resource key through to
MetricPinStore, and pinned group counts use the same resource-aware lookup.Existing settings databases are migrated without guessing a resource. Legacy rows gain a nullable resource identity and remain explicit global pins, preserving their previous behavior. Unpinning a migrated global pin removes that global record; pinning again creates a resource-scoped record.
User-facing usage
Users can pin an instrument card for one resource without implicitly pinning matching cards from other resources or replicas. The selected resource remains pinned after dashboard reloads.
Screenshots / Recordings
Validation
dotnet test --project tests\Aspire.Dashboard.Tests\Aspire.Dashboard.Tests.csproj --no-launch-profile -- --filter-class "*.MetricPinStoreTests" --filter-class "*.DashboardSettingsDatabaseTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"- passed, 9 tests.dotnet test --project tests\Aspire.Dashboard.Components.Tests\Aspire.Dashboard.Components.Tests.csproj --no-launch-profile -- --filter-class "*.MetricsTests" --filter-not-trait "quarantined=true" --filter-not-trait "outerloop=true"- passed, 43 tests.Fixes #21710
Checklist
<remarks />and<code />elements on your triple slash comments?