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.

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

Jump to bottom

Add support for showing custom resources in the dashboard #2390

Merged
merged 10 commits into from
Feb 28, 2024
Merged

Add support for showing custom resources in the dashboard #2390

merged 10 commits into from
Feb 28, 2024

Conversation

davidfowl
Copy link
Member

@davidfowl davidfowl commented Feb 24, 2024 •
edited
Loading

  • Added DashboardAnnotation which allows changing a dashboard state. There are 2 methods, one to create the initial state from resource annotations and another to send updates to the dashboard. These are immutable snapshots to avoid thread safety issues.
  • Show all resources based on the app model before watching dcp for updates.
custom-resource2.mp4

Left TODO:

  • Replicas are broken on the resources view with this change. We need to delete the existing project resources if there are replicas.
  • Tests?

Follow up work to enable logging.

Fixes #436

Microsoft Reviewers: Open in CodeFlow

@davidfowl davidfowl added this to the preview 5 (Apr) milestone Feb 24, 2024
drewnoakes
drewnoakes reviewed Feb 24, 2024
View reviewed changes
Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments from my phone while waiting for a plane, so apologies for the lack of depth here.

src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Outdated Show resolved Hide resolved
src/Aspire.Dashboard/Components/ResourcesGridColumns/SourceColumnDisplay.razor Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardAnnotation.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardResoruceExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardResoruceExtensions.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardAnnotation.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/GenericResourceSnapshot.cs Outdated Show resolved Hide resolved
@davidfowl davidfowl mentioned this pull request Feb 24, 2024
Closed
mitchdenny
mitchdenny reviewed Feb 26, 2024
View reviewed changes
src/Aspire.Hosting/Dashboard/DcpDataSource.cs
ResourceSnapshot? snapshot = default;

// If the resource is a redirect, we want to create a snapshot for the resource it redirects to.
if (resource.TryGetLastAnnotation<ConnectionStringRedirectAnnotation>(out var redirectAnnotation))
Copy link
Member

Choose a reason for hiding this comment

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

We should change the name of this annotation if we are going to use it in this way ... its scope is now much broader than connection strings.

@davidfowl davidfowl mentioned this pull request Feb 26, 2024
Closed
6 tasks
@davidfowl
Add support for showing custom resources in the dashboard
41570c7 
- Added DashboardAnnotation which allows changing a dashboard context. This context consists of properties, urls and the resource state.
- Show all resources based on the app model before watching dcp for updates.

Added a model for upates
- Use a snapshotting model for dashboard updates

Update the previous state

Clean up how we resolve projects from DCP resources

Some feedback
- Make the initial snapshot state take computed values from well known annotations.

Fixed typos

Small change

Added a public DashboardKnownProperties

Added playground for testing custom resource API

Remove manifest

Handle missing parameter values

Fix configuration

Apply feedback and made more changes
- Clean up te threading model. There can only be a single producer and consumer. The producer is responsible to keeping the state around if small mutations are to be made.
- Keep the resource type filter working

Remove exit code, fix create timestamp

ExecutionContext not used

Flow the cancellation token

Move to single loop

Remove logs hack for now
@davidfowl davidfowl force-pushed the davidfowl/dashboard-custom-resources branch from f30595c to 41570c7 Compare February 27, 2024 19:13
@davidfowl davidfowl marked this pull request as ready for review February 27, 2024 21:30
karolz-ms
karolz-ms approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of suggestions

playground/CustomResources/CustomResources.AppHost/TestResource.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardAnnotation.cs Outdated Show resolved Hide resolved
mitchdenny
mitchdenny approved these changes Feb 28, 2024
View reviewed changes
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM. Some reservations about the annotation name. But overall this is a good change (and much needed).

JamesNK
JamesNK reviewed Feb 28, 2024
View reviewed changes
src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Outdated Show resolved Hide resolved
src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Outdated Show resolved Hide resolved
src/Aspire.Dashboard/Components/Pages/Resources.razor.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/ConsoleLogPublisher.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DashboardAnnotation.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DcpDataSource.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dashboard/DcpDataSource.cs Outdated Show resolved Hide resolved
@davidfowl
PR feedback
0c86f71 
- Rename the annotation to CustomResourceAnnotation
- Simplify the UI
- Suppress async warning
@davidfowl
Concurrent dictionaries everywhere
1f2b625 
- Does not remove resource types from the filter
@davidfowl davidfowl enabled auto-merge (squash) February 28, 2024 02:32
@davidfowl
Copy link
Member Author

Thanks for the review all! I will follow up with a logs PR.

@davidfowl davidfowl merged commit 70e1d1b into main Feb 28, 2024
8 checks passed
@davidfowl davidfowl deleted the davidfowl/dashboard-custom-resources branch February 28, 2024 03:10
@davidfowl davidfowl mentioned this pull request Feb 28, 2024
Closed
radical pushed a commit to radical/aspire that referenced this pull request Feb 28, 2024
@davidfowl @radical
Add support for showing custom resources in the dashboard (dotnet#2390)
b4c11f2 
* Add support for showing custom resources in the dashboard
- Added CustomResourceAnnotation which exposes methods for getting the initial snapshot and changing resource snapshots. These changes show up in the dashboard.
- Show all resources based on the app model before watching dcp for updates. We put a placeholder in the dashboard in the "Starting" state and then remove it when DCP updates come in.
@paulomorgado paulomorgado mentioned this pull request Feb 29, 2024
Open
@github-actions github-actions bot locked and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@drewnoakes drewnoakes drewnoakes left review comments

@JamesNK JamesNK JamesNK approved these changes

@mitchdenny mitchdenny mitchdenny approved these changes

@karolz-ms karolz-ms karolz-ms approved these changes

Assignees

@davidfowl davidfowl

Labels
area-dashboard
Projects
None yet
Milestone
preview 5 (Apr)
Development

Successfully merging this pull request may close these issues.

Dashboard should show custom resources
5 participants
@davidfowl @JamesNK @drewnoakes @mitchdenny @karolz-ms