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

Dashboard should request log streams only when the user gestures to display them (for given resource) #2789

Closed
karolz-ms opened this issue Mar 11, 2024 · 1 comment · Fixed by #3235
Closed

Dashboard should request log streams only when the user gestures to display them (for given resource) #2789

karolz-ms opened this issue Mar 11, 2024 · 1 comment · Fixed by #3235
Assignees
@eerhardt
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Milestone
preview 6 (Apr)

Comments

@karolz-ms
Copy link
Member

With #2731 the dashboard requests log streams for all application resources at the startup or shortly after. This is not efficient, especially for Containers, where each log stream is associated with a separate container orchestrator (CLI) running in the background.
@karolz-ms karolz-ms added this to the preview 5 (Apr) milestone Mar 11, 2024
@davidfowl davidfowl added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-dashboard untriaged labels Mar 11, 2024
@davidfowl davidfowl assigned eerhardt and unassigned davidfowl Mar 27, 2024
@davidfowl
Copy link
Member

Made progress on this here https://github.com/dotnet/aspire/tree/davidfowl/on-demand-logs. Not seeing DCP kill the docker logs command on cancellation.

@eerhardt eerhardt mentioned this issue Mar 28, 2024
Merged
eerhardt added a commit that referenced this issue Apr 2, 2024
@eerhardt @davidfowl
Dashboard should request log streams only when the user gestures to d…
a75513f 
…isplay them (#3235)

Change the AppHost to only subscribe to DCP's logs when there is a subscriber for the logs.

Fix #2789

* Show logs

* Remove the "StreamingLogger" because that causes subsequent subscribers to not see what was already written.

Fix duplicate logs issue by clearing out the backlog when the last subscriber leaves.

* Fix a concurrency issue with the backlog. Ensure the backlog is only snap shotted after subscribing to the log.

Fix existing tests for new functionality and add an additional test.

* PR feedback

Only clear the backlog on containers and executables.

* PR feedback.

Move lock out of async iterator.

* Clean up code.

- Remove count and instead check if the delegate is null to indicate whether there are subscribers or not.
- Remove the separate IAsyncEnumerable classes and just use `async IAsyncEnumerable` methods.

* Fix a race at startup when logs aren't available.

Employ a 2nd loop that listens for both "has subscribers" and "logs available".

* Simplify ResourceNotificationService.WatchAsync.

* Fix test build

* Address PR feedback

- Set SingleReader=true on the logInformationChannel.
- Add comment and assert that LogsAvailable can only turn true and can't go back to false.

---------

Co-authored-by: David Fowler <davidfowl@gmail.com>
eerhardt added a commit to eerhardt/aspire that referenced this issue Apr 2, 2024
@eerhardt @davidfowl
Dashboard should request log streams only when the user gestures to d…
797263d 
…isplay them (dotnet#3235)

Change the AppHost to only subscribe to DCP's logs when there is a subscriber for the logs.

Fix dotnet#2789

* Show logs

* Remove the "StreamingLogger" because that causes subsequent subscribers to not see what was already written.

Fix duplicate logs issue by clearing out the backlog when the last subscriber leaves.

* Fix a concurrency issue with the backlog. Ensure the backlog is only snap shotted after subscribing to the log.

Fix existing tests for new functionality and add an additional test.

* PR feedback

Only clear the backlog on containers and executables.

* PR feedback.

Move lock out of async iterator.

* Clean up code.

- Remove count and instead check if the delegate is null to indicate whether there are subscribers or not.
- Remove the separate IAsyncEnumerable classes and just use `async IAsyncEnumerable` methods.

* Fix a race at startup when logs aren't available.

Employ a 2nd loop that listens for both "has subscribers" and "logs available".

* Simplify ResourceNotificationService.WatchAsync.

* Fix test build

* Address PR feedback

- Set SingleReader=true on the logInformationChannel.
- Add comment and assert that LogsAvailable can only turn true and can't go back to false.

---------

Co-authored-by: David Fowler <davidfowl@gmail.com>
@eerhardt eerhardt mentioned this issue Apr 2, 2024
Merged
radical pushed a commit to radical/aspire that referenced this issue Apr 3, 2024
@eerhardt @davidfowl @radical
Dashboard should request log streams only when the user gestures to d…
20f5455 
…isplay them (dotnet#3235)

Change the AppHost to only subscribe to DCP's logs when there is a subscriber for the logs.

Fix dotnet#2789

* Show logs

* Remove the "StreamingLogger" because that causes subsequent subscribers to not see what was already written.

Fix duplicate logs issue by clearing out the backlog when the last subscriber leaves.

* Fix a concurrency issue with the backlog. Ensure the backlog is only snap shotted after subscribing to the log.

Fix existing tests for new functionality and add an additional test.

* PR feedback

Only clear the backlog on containers and executables.

* PR feedback.

Move lock out of async iterator.

* Clean up code.

- Remove count and instead check if the delegate is null to indicate whether there are subscribers or not.
- Remove the separate IAsyncEnumerable classes and just use `async IAsyncEnumerable` methods.

* Fix a race at startup when logs aren't available.

Employ a 2nd loop that listens for both "has subscribers" and "logs available".

* Simplify ResourceNotificationService.WatchAsync.

* Fix test build

* Address PR feedback

- Set SingleReader=true on the logInformationChannel.
- Add comment and assert that LogsAvailable can only turn true and can't go back to false.

---------

Co-authored-by: David Fowler <davidfowl@gmail.com>
eerhardt added a commit that referenced this issue Apr 4, 2024
@eerhardt @davidfowl
Dashboard should request log streams only when the user gestures to d…
14b23a1 
…isplay them (#3235) (#3343)

Change the AppHost to only subscribe to DCP's logs when there is a subscriber for the logs.

Fix #2789

* Show logs

* Remove the "StreamingLogger" because that causes subsequent subscribers to not see what was already written.

Fix duplicate logs issue by clearing out the backlog when the last subscriber leaves.

* Fix a concurrency issue with the backlog. Ensure the backlog is only snap shotted after subscribing to the log.

Fix existing tests for new functionality and add an additional test.

* PR feedback

Only clear the backlog on containers and executables.

* PR feedback.

Move lock out of async iterator.

* Clean up code.

- Remove count and instead check if the delegate is null to indicate whether there are subscribers or not.
- Remove the separate IAsyncEnumerable classes and just use `async IAsyncEnumerable` methods.

* Fix a race at startup when logs aren't available.

Employ a 2nd loop that listens for both "has subscribers" and "logs available".

* Simplify ResourceNotificationService.WatchAsync.

* Fix test build

* Address PR feedback

- Set SingleReader=true on the logInformationChannel.
- Add comment and assert that LogsAvailable can only turn true and can't go back to false.

---------

Co-authored-by: David Fowler <davidfowl@gmail.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@eerhardt eerhardt

Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Milestone
preview 6 (Apr)
Development

Successfully merging a pull request may close this issue.

Dashboard should request log streams only when the user gestures to display them dotnet/aspire
3 participants
@davidfowl @eerhardt @karolz-ms