Dashboard should request log streams only when the user gestures to display them #3235
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rs to not see what was already written. Fix duplicate logs issue by clearing out the backlog when the last subscriber leaves.
dotnet-issue-labeler
bot
added
the
area-app-model
|
What happens to historical logs? For example, will requesting the console logs for an app show all longs since the app started? (if less than the limit) |
…snap shotted after subscribing to the log. Fix existing tests for new functionality and add an additional test.
eerhardt
force-pushed
the
on-demand-logs
branch
from
56647c5
to
cae7d06
Compare
The "backlog" gets cleared after the last subscriber to a resource leaves. Later, when a "new" subscriber comes along, they are all replayed from DCP because we call So even if no one was subscribed for a while, the whole logs come back once someone does. |
|
One small race is that starting logs before the container is ready, it won’t show up. Super tight race and I don’t know if it’ll be worth complicating the code but I thought I’d let you know |
davidfowl
reviewed
Mar 28, 2024
Only clear the backlog on containers and executables.
Can you describe this a bit more? When what is starting the logs before the container is ready? |
Move lock out of async iterator.
- 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.
Employ a 2nd loop that listens for both "has subscribers" and "logs available".
This didn't turn out to be too bad. I added another loop that joined the "has subscribers" and "logs available" information, and only starts the logs once both are true. |
eerhardt
force-pushed
the
on-demand-logs
branch
from
119d860
to
3de5030
Compare
davidfowl
reviewed
Mar 30, 2024
davidfowl
reviewed
Mar 30, 2024
davidfowl
approved these changes
Mar 30, 2024
- Set SingleReader=true on the logInformationChannel. - Add comment and assert that LogsAvailable can only turn true and can't go back to false.
eerhardt
added a commit
to eerhardt/aspire
that referenced
this pull request
Apr 2, 2024
…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 <[email protected]>
radical
pushed a commit
to radical/aspire
that referenced
this pull request
Apr 3, 2024
…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 <[email protected]>
eerhardt
added a commit
that referenced
this pull request
Apr 4, 2024
…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 <[email protected]>
Closed
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change the AppHost to only subscribe to DCP's logs when there is a subscriber for the logs.
Fix #2789
Microsoft Reviewers: Open in CodeFlow