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

Do not send Heap Stats to the LSP log #3111

Merged
merged 4 commits into from
Aug 18, 2022
Merged

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Aug 17, 2022

It interferes with the ability of lsp-test to detect timeouts in e.g. benchmarks

The assumption is that logging them to stderr serves the purpose they
were designed for.

It interferes with the ability of lsp-test to detect timeouts in e.g. benchmarks

The assumption is that logging them to stderr serves the purpose they
were designed for.
@michaelpj
Copy link
Collaborator

Would it make sense to just disable the client logs when doing tests or benchmarks?

@pepeiborra
Copy link
Collaborator Author

Would it make sense to just disable the client logs when doing tests or benchmarks?

It's easy to do and I don't have a strong opinion

@pepeiborra pepeiborra force-pushed the silence-heap-stats-lsp branch from 9226cb8 to 43586ab Compare August 17, 2022 21:27
@pepeiborra pepeiborra force-pushed the silence-heap-stats-lsp branch from 43586ab to eb32021 Compare August 17, 2022 21:35
Copy link
Collaborator

@kokobd kokobd left a comment

Choose a reason for hiding this comment

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

Would it make sense to just disable the client logs when doing tests or benchmarks?

It's easy to do and I don't have a strong opinion

Then it looks good to me. Some users might be using this log as a quick measure to check memory consumption.

@pepeiborra
Copy link
Collaborator Author

Would it make sense to just disable the client logs when doing tests or benchmarks?

It's easy to do and I don't have a strong opinion

Then it looks good to me. Some users might be using this log as a quick measure to check memory consumption.

This PR doesn't remove the log from stderr, only from the LSP stream. We only started emitting those around 3 months ago so I doubt any users depend on them.

@pepeiborra pepeiborra enabled auto-merge (squash) August 18, 2022 05:35
@pepeiborra pepeiborra merged commit c9ed045 into master Aug 18, 2022
@michaelpj
Copy link
Collaborator

I just think it makes our logging less consistent, which is confusing in future. But 🤷

sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request Sep 12, 2022
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