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

Merge feature/dashboard-oop to main #1774

Merged
merged 25 commits into from
Jan 24, 2024
Merged

Merge feature/dashboard-oop to main #1774

merged 25 commits into from
Jan 24, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Jan 23, 2024

Fixes #1003
Fixes #851
Fixes #1753
Fixes #1688
Fixes #1687
Fixes #1686
Fixes #1639

This PR merges the feature branch for OOP dashboard work into main.

It will remain in draft until it is ready to merge.

The checklist of outstanding work (may be incomplete, but to be built up over the next day or two):

Must have

  • Validate manifest generation, as that has broken when we've made similar changes in the past
  • Pass OLTP URL from user's AppHost project's environment (via launchSettings.json) to the dashboard process's environment when launching it
  • End to end testing
  • Validate that all claimed bug fixes are true
  • App model awaits dashboard readiness before starting other apps, so no telemetry is lost
  • Workload passes executable via dashboard attribute. Must either change to the DLL, or change the ApplicationExecutor to work with executables (not dotnet <dll>).

Candidates

image

Microsoft Reviewers: Open in CodeFlow

joperezr and others added 9 commits January 18, 2024 15:48
* Run dashboard out-of-process

This change causes the dashboard to be run as an orchestrated project, just like any other Aspire resource.

To achieve this, we:

- Break the project dependency from AppHost upon Dashboard.
  - Until we have support for dotnet tool resources, having the sample projects depend upon the dashboard directly, and register them with the DAB.
  - Copy `TimestampParser.DisplayFormat` into `Aspire.Hosting.Dashboard.ConsoleLogsConfigurationExtensions` where it was used, and add comments to keep them in sync.
- Have the dashboard wire up its own web application.
  - Remove `DashboardWebApplicationHost`.
  - Remove dashboard-specific code from `DistributedApplicationBuilder`.
  - Update URLs to resources defined in the dashboard. They no longer need (or work with) the `_content/Aspire.Dashboard/` prefix.
  - Remove workarounds/configuration to support static web assets, as the new structure supports this automatically.
  - Link `KnownProperties` and `KnownResourceTypes` into the hosting project, so both projects can share the definition without needing an assembly reference.
- Change the MSBuild SDK for the dashboard from Razor Class Library to Web project.
  - Remove explicit framework reference.
  - Remove redundant using directives (the new SDK has different implicit usings).
  - Make the dashboard an executable.
  - Update the CSS bundle file name (from `Aspire.Dashboard.bundle.scp.css` to `Aspire.Dashboard.styles.css`)

This work is incomplete for a few reasons:

1. Aspire users won't be able to create a project reference to the dashboard. That only works in this solution. We need support for resources defined via `dotnet tool` packages before we can fix that.
2. We don't launch a browser to the dashboard any more. You have to find the URL in the logs and launch it.

We will need to address those issues before the changes here can be merged.

---

To test this locally, run any of the sample applications. To find the dashboard's URL, navigate to `%TEMP%` and find the relevant log files. For example, `%TEMP%\aspire.oweozx1d.bmh\dashboard-5buv5so_out_eedbad51-0294-4ff8-9add-001f27499206` (your IDs will be different). The first log message should read _Now listening on: https://localhost:49337_ or similar. Open that URL in a browser.

* Make dashboard packable as a dotnet tool

Also adds a readme explaining dashboard configuration, and sets some metadata on the package for the icon, readme, etc.
…e/dashboard-oop

[automated] Merge branch 'main' => 'feature/dashboard-oop'
…ry for dashboard (#1705)

* Add initial workload targets and manifest entry for dashboard

* Do a better job handling trailing slashes for workload tool paths

* Update based on PR feedback

* Publishing as a tool by default breaks the workload build

* Remove extra parenthesis from import condition

* Updated workload package name
* Expose resource service URIs on DashboardServiceHost

The resource service will no longer bind to a default, hardcoded port. This encourages port collisions. Instead, when no URL is set in the environment, a port is chosen dynamically.

When the app model launches the dashboard, it must pass the URIs exposed via `DashboardServiceHost.GetUrisAsync` via the `DOTNET_DASHBOARD_GRPC_ENDPOINT_URL` environment variable to the spawned instance. That work is happening concurrently, and will use this API is a subsequent change.

* Resource service listens on a single URI which must be loopback

* Require parsed URI to be absolute
…the workload (#1767)

* Ensure the dashboard actually builds as a package for the workload

* Publish then pack the dashboard as part of workload build
@drewnoakes drewnoakes added area-dashboard area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Jan 23, 2024
@drewnoakes drewnoakes added this to the preview 3 (Feb) milestone Jan 23, 2024
mitchdenny and others added 10 commits January 23, 2024 20:13
* Run dashboard out-of-process

This change causes the dashboard to be run as an orchestrated project, just like any other Aspire resource.

To achieve this, we:

- Break the project dependency from AppHost upon Dashboard.
  - Until we have support for dotnet tool resources, having the sample projects depend upon the dashboard directly, and register them with the DAB.
  - Copy `TimestampParser.DisplayFormat` into `Aspire.Hosting.Dashboard.ConsoleLogsConfigurationExtensions` where it was used, and add comments to keep them in sync.
- Have the dashboard wire up its own web application.
  - Remove `DashboardWebApplicationHost`.
  - Remove dashboard-specific code from `DistributedApplicationBuilder`.
  - Update URLs to resources defined in the dashboard. They no longer need (or work with) the `_content/Aspire.Dashboard/` prefix.
  - Remove workarounds/configuration to support static web assets, as the new structure supports this automatically.
  - Link `KnownProperties` and `KnownResourceTypes` into the hosting project, so both projects can share the definition without needing an assembly reference.
- Change the MSBuild SDK for the dashboard from Razor Class Library to Web project.
  - Remove explicit framework reference.
  - Remove redundant using directives (the new SDK has different implicit usings).
  - Make the dashboard an executable.
  - Update the CSS bundle file name (from `Aspire.Dashboard.bundle.scp.css` to `Aspire.Dashboard.styles.css`)

This work is incomplete for a few reasons:

1. Aspire users won't be able to create a project reference to the dashboard. That only works in this solution. We need support for resources defined via `dotnet tool` packages before we can fix that.
2. We don't launch a browser to the dashboard any more. You have to find the URL in the logs and launch it.

We will need to address those issues before the changes here can be merged.

---

To test this locally, run any of the sample applications. To find the dashboard's URL, navigate to `%TEMP%` and find the relevant log files. For example, `%TEMP%\aspire.oweozx1d.bmh\dashboard-5buv5so_out_eedbad51-0294-4ff8-9add-001f27499206` (your IDs will be different). The first log message should read _Now listening on: https://localhost:49337_ or similar. Open that URL in a browser.

* Make dashboard packable as a dotnet tool

Also adds a readme explaining dashboard configuration, and sets some metadata on the package for the icon, readme, etc.

* Launching dashboard via DCP executable.

* Work in progress checkin.

* Remove unnecessary environment sets.

* Move dashboard path resolution to dcp options.

* PR feedback.

* Read ASPNETCORE_ENVIRONMENT ... from environment.

* Update src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs

Co-authored-by: Drew Noakes <[email protected]>

* Update src/Aspire.Hosting/Dashboard/DashboardServiceHost.cs

Co-authored-by: Drew Noakes <[email protected]>

* Exclude dashboard resources from being watched.

* Filtering dashboard resources from view.

* Revert dashboard servicehost to parent branch state.

* Changes after integrating feature branch.

* Fix unit tests

---------

Co-authored-by: Drew Noakes <[email protected]>
* Formatting

Addresses diagnostics shown by the IDE. If we don't want these changes, we should reconfigure or disable those diagnostics.

* Rename method and log if delays are observed

* Replace `Trace.WriteLine` with `_logger.LogWarning`
* Add KnownResourceNames

* Ensure dashboard is started first

When F5'ing one of the playground solutions in the dotnet/aspire repo, the dashboard is added as the last resource. This means it starts after all other resources, which is not what we want. This change ensures it is always started first.
* Add wait for dashboard start.

* WIP to merge with parent branch.

* Wait for dashboard if it is a Aspire resource.

* Add dashboard setup across playground.

* Update src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Co-authored-by: Drew Noakes <[email protected]>

* Add short delay in polling loop.

* Fix up get request.

---------

Co-authored-by: Drew Noakes <[email protected]>
…cking (#1771)

* Publish dashboard for specific RIDs before packing

* Add comments explaining the need to keep runtime lists in sync

* Remove unused item for icon

* Ensure it is clear the runtimes list matches the supported runtimes from DCP

* Allow parallel publishing
…ects (#1799)

* Update attribute handling for playground projects

* Update comments on default path to dashboard
* Support starting dashboard as DLL or executable

The workload ships an executable, but `ApplicationExecutor` was written to run a DLL via `dotnet <dll>`.

This change checks whether the dashboard is a DLL or not, and configures the executable spec accordingly.

* Fix compile error

Co-authored-by: David Negstad <[email protected]>

---------

Co-authored-by: David Negstad <[email protected]>
Copy link
Member

@JamesNK JamesNK left a comment

Choose a reason for hiding this comment

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

The PR is a draft and things are still in progress, but since time is important, I've added some comments.

Edit: I'll just make a PR to the branch myself to fix these.

src/Aspire.Dashboard/Program.cs Show resolved Hide resolved
src/Aspire.Dashboard/ConsoleLogs/TimestampParser.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved
src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated Show resolved Hide resolved
* Fix dashboard startup ordering.

* Workaround pathing bug.

* Fix up AspireDashboardDir (keep it in Directory.Build.props in repo root).
@drewnoakes drewnoakes marked this pull request as ready for review January 24, 2024 05:11
@JamesNK
Copy link
Member

JamesNK commented Jan 24, 2024

Now that the dashboard runs in a separate process, how do we view console logs for the dashboard in local development?

@mitchdenny
Copy link
Member

Now that the dashboard runs in a separate process, how do we view console logs for the dashboard in local development?

Uncomment this line:

// "DOTNET_ASPIRE_SHOW_DASHBOARD_RESOURCES": "true"

Ideally you will also have the dashboard added as a resource (the default in the source) so that the debugger will get attached as well.

@JamesNK
Copy link
Member

JamesNK commented Jan 24, 2024

Ok. Shouldn't it always be uncommented for our sandbox projects?

@mitchdenny
Copy link
Member

Ok. Shouldn't it always be uncommented for our sandbox projects?

Yeah that is fair. Did you want to put it in your catch all PR?

@JamesNK
Copy link
Member

JamesNK commented Jan 24, 2024

I'll do it in my catch all 👻

@JamesNK
Copy link
Member

JamesNK commented Jan 24, 2024

I now see the dashboard in the dashboard 🔥 Good: Console logs are available. Bad: Other telemetry (structured logs, tracing, metrics) isn't.

We don't need it for P3, but it would be great if the dashboard sent telemetry about the dashboard to the dashboard, so it is visible in the dashboard.

@drewnoakes
Copy link
Member Author

The test Aspire.Azure.Storage.Queues.Tests.ConformanceTests.TracingEnablesTheRightActivitySource fails intermittently but fairly regularly.

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This hasn't shipped anywhere yet, so it's fine to rename it now.
@drewnoakes
Copy link
Member Author

Results from testing.

eShopLite

  • Manifest generation completes
  • F5 works

image

orleans

  • Manifest generation completes
  • F5 works

image

dapr

Note

Untested as I don't have C:\dapr\dapr.exe.

  • Manifest generation completes
  • F5 works

CosmosEndToEnd

  • Manifest generation completes
  • F5 works

image

Results

I'm unclear why this last screenshot doesn't show the aspire-dashboard process, so filed:

@drewnoakes
Copy link
Member Author

@drewnoakes drewnoakes merged commit 188074a into main Jan 24, 2024
8 checks passed
@drewnoakes drewnoakes deleted the feature/dashboard-oop branch January 24, 2024 12:07
Comment on lines +3 to +5
## As a dotnet tool

The dashboard can be packaged and executed as a `dotnet tool`.

Choose a reason for hiding this comment

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

Do you intend to publish the dashboard as a .NET tool to NuGet Gallery, or does this README mean I should build the dashboard locally if I want it as a tool? It looks like the tool packaging was disabled in commit 7bf1c29 of #1705.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies, the docs weren't updated when the tool support was removed.

We removed it because we no longer needed it for our scenario and (from memory) it conflicted with other packaging requirements.

Do you have a use case for a tool? If so, could you file an issue explaining how you'd use it?

Choose a reason for hiding this comment

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

I'm reimplementing a two-decades-old service in .NET, currently multitargeting net462 and net8.0. It would benefit from a web dashboard showing the status of its components, but I'd like to isolate the web stuff in a separate process, to minimize the risk that problems in it affect the service itself. I hoped that, if I instrument the service with the Activity and Meter APIs and add OpenTelemetry.Exporter.OpenTelemetryProtocol, then the Aspire dashboard would be able to display the status, and I wouldn't need to implement my own dashboard. Alternatively, I could use some other third-party dashboard, not necessarily in .NET.

I see an Aspire.Dashboard.exe file was installed as part of the Aspire.Dashboard.Sdk.win-x64 pack. I'll play around with that and see whether it is at all suitable for my purposes; if not, then there's no reason to request a tool package for 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.

I think you should be able to get that executable working and not need a dotnet tool. If not, please open a new issue.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.