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

Implement logging for debug adapter clients #45

Merged
merged 9 commits into from
Oct 21, 2024

Conversation

bbutkovic
Copy link

@bbutkovic bbutkovic commented Oct 4, 2024

debugger-log-view.mov

@bbutkovic bbutkovic marked this pull request as ready for review October 19, 2024 12:26
@RemcoSmitsDev
Copy link
Owner

I think should consider to always enable RPC messages, because for connecting to a debug adapter the initial request is crucial. Right now we cannot see them, because we did not enable RPC messages yet.

@RemcoSmitsDev
Copy link
Owner

I also think we should consider refactoring how we pass around the transport params and IO params, maybe we should move the connect tcp/stdio methods to the transport layer. So each debug adapter does not have to care about how they should return the correct transport + log IO params.

@RemcoSmitsDev RemcoSmitsDev self-requested a review October 19, 2024 22:15
@Anthony-Eid
Copy link
Collaborator

This looks good! Maybe it would be beneficial to use create a Model instead of using Mutex to store the log messages. That why it would be more inline with how the rest of the Zed code base handles stuff. However, that's not a blocking issue and can be changed in the future.

@RemcoSmitsDev RemcoSmitsDev removed their request for review October 20, 2024 08:36
@bbutkovic
Copy link
Author

Thanks for the input! I'll make the changes and let you know when it's finished.

I also think we should consider refactoring how we pass around the transport params and IO params, maybe we should move the connect tcp/stdio methods to the transport layer. So each debug adapter does not have to care about how they should return the correct transport + log IO params.

I think that's a good idea. Do you want me to make this change in my branch?

I think should consider to always enable RPC messages, because for connecting to a debug adapter the initial request is crucial. Right now we cannot see them, because we did not enable RPC messages yet.

Just so I'm sure I got it right, we should get rid of the option to enable/disable RPC logs, right? Or should we enable it by default and allow people to disable it? I think if we're going to default to it being enabled we can just get rid of the toggle altogether.

…or logging. So not each debug adapters have to map the AdapterLogIO fields. I also removed some specific when has logs from the client, because the client is not responsible for that. Removed an not needed/duplicated dependency Fix formatting & clippy

This cleans up the way we pass through the input and output readers for logging. So not each debug adapters have to map the AdapterLogIO fields.

I also removed some specific when has logs from the client, because the client is not responsible for that.

Removed an not needed/duplicated dependency

Fix formatting & clippy
RemcoSmitsDev and others added 6 commits October 20, 2024 20:44
Co-Authored-By: Anthony Eid <hello@anthonyeid.me>
Previously, RPC messages were only stored when explicitly enabled, which occurred after the client was already running. This approach prevented debugging of requests sent during the initial connection period. By always enabling RPC messages, we ensure that all requests, including those during the connection phase, are captured and available for debugging purposes.

This could help use debug when someone has troble getting a debug starting. This improvement could be particularly helpful in debugging scenarios where users encounter issues during the initial connection or startup phase of their debugging sessions.
@RemcoSmitsDev
Copy link
Owner

Thanks a lot🔥

@RemcoSmitsDev RemcoSmitsDev merged commit 43ea3b4 into RemcoSmitsDev:debugger Oct 21, 2024
2 of 6 checks passed
@RemcoSmitsDev RemcoSmitsDev deleted the debugger-log branch October 21, 2024 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants