-
Notifications
You must be signed in to change notification settings - Fork 462
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
Use streaming to get logs for Executables and Containers #2435
Conversation
DCP insertion has started. Marking as ready for review |
This will resolve https://github.com/microsoft/usvc/issues/8 BTW |
New DCP release is in #2442. This is ready to merge now (once approved). |
@karolz-ms I'm going to test this out. In the interest of time, do you mind if I make changes to your branch if I find things to change? |
Not at all. Appreciate the help. Note you need new DCP |
I got this error if I view logs for a resource as the app is starting up:
The user experience should be they click view logs for a resource that is starting, and logs start displaying when they become available. I don't know the best implementaiton for that. Perhaps some retrying to happen for a 404 error to give DCP a chance to finish starting the resource up. |
The error above is probably due to outdated DCP version. That said, there is known limitation in the current implementation, which is, if a Container/Executable is just created and starting, logs will not be available (will show up as empty). We will fix that with upcoming change in DCP, there will be no client change required. |
2089f06
to
2a3e126
Compare
Seems to be working well for me locally, although on an unrelated note I do notice that the dashboard is logging cancellations every time you switch log views ( |
Yea that’s a common dashboard error |
Did we fix all the things? Is this ready to go? |
There are some issues:
I'm looking at these both. The first issue will be a separate PR. The second issue will be added to this PR (I'll do it today). |
Feels like we should merge this PR with the bool flipped (back to the current impl). Work on the issues and flip to back if we’re happy with the progress. |
There is a fairly long delay (5-10 seconds) between logs being written and them showing up in the dashboard. I have some code that writes to
You can see in the recording that the code executes immediately ( I repeated this test on the old console logs code. Previous console logging displayed updates immediately. |
Good feedback, thanks James. I talked to David and the current proposal is to have the new implementation be an opt-in behind an env var and pursue this for Aspire P5. |
2a3e126
to
383700f
Compare
Replaces #2414
This PR takes advantage of the new DCP feature: streaming logs. Thus, the dashboard service is relieved from the task of watching files and interacting with container orchestrator (Docker, Podman, etc) directly.
Marking as draft PR because it requires a DCP change, which @danegsta and @bwateratmsft are currently reviewing with considerable frenzy.
Microsoft Reviewers: Open in CodeFlow