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

x/tools/gopls: replace gopls.add_telemetry with a hidden gopls add_telemetry command #64428

Closed
hyangah opened this issue Nov 28, 2023 · 6 comments
Assignees
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Nov 28, 2023

We added gopls.add_telemetry to allow vscode-go to collect editor metrics through gopls. However, this LSP command works only gopls is up and running, which limits its utility. Instead, I propose to have a gopls subcommand, and vscode-go invokes that as a separate process. That will allow vscode-go to collect telemetry even when the language server is disabled.

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Nov 28, 2023
@gopherbot gopherbot added this to the Unreleased milestone Nov 28, 2023
@gopherbot
Copy link

Change https://go.dev/cl/546419 mentions this issue: gopls/internal/cmd: add 'execute' command

@adonovan adonovan self-assigned this Nov 30, 2023
@hyangah
Copy link
Contributor Author

hyangah commented Dec 1, 2023

Thank you for the CL, but as I said in the meeting, I want to have a light-weight hidden command for this and discuss general gopls execute in a separate issue. This issue also asks to remove the gopls.add_telemetry LSP command. What I am thinking is just set up a line scanner on stdin, parses name, count, and call the telemetry counter.Inc call. No JSON encoding/decoding or LSP dependency.

I am still not sure about gopls execute subcommand. Who are the target users and when they want to use the interface designed for LSP outside LSP session?

  • Commands like gopls.check_upgrades, gopls.run_vulncheck, gopls.toggle_gc_details, gopls.reset_go_mod_diagnostics are used to trigger diagnostics or turn off diagnostics. Without capturing the followup diagnostics messages, exposing them is not doing any useful work.

  • Commands like gopls.vendor, gopls.generate, gopls.run_tests, gopls.run_go_work, gopls.tidy, gopls.list_imports, gopls.edit_go_directive have a better and easier-to-use alternative, go.

  • Commands like gopls.start_profile, gopls.stop_profile, gopls.start_debugging, gopls.mem_stats aren't useful outside server mode.

  • Commands like gopls.apply_fix, gopls.fetch_vulncheck_results aren't working without prior diagnostics.

  • Commands like gopls.change_signature or other refactoring commands may be useful with better command line interface. (e.g. should users provide UTF16 position info?)

Most commands are designed to be used along with Code Lens, Code Action, and Diagnostics. That allowed us to change API of the custom commands (e.g. we can change gopls.tidy arg type or even the command name without worrying about backwards/forwards compatibility). We made a few exceptions for simple commands, but they are mostly experimental or hidden for advanced usage. I am afraid that we are moving the bar by exposing them through CLI used by human directly.

@adonovan
Copy link
Member

adonovan commented Dec 1, 2023

The gopls CLI is no more supported or stable an interface than the various commands supported through the ExecuteCommand service method, so I'm not worried about increasing exposure through the CLI tool. You're right that many of the commands are low-level, internal, or redundant with other features, but they are often the only the way (short of writing code) to test an LSP-level feature in isolation while trying to minimize a reproducible test case for a user-reported issue---that's the main reason I've been adding commands recently, and I have wanted this 'gopls execute' on several occasions.

At some point we should think about designing the command-line interface that we really want and committing to support it, because gopls is currently the only way for Go users to script various operations (such as renaming) that used to have a dedicated command-line tool. But that's not on our plans yet. When that happens, any existing command that we don't want to publish can be pushed beneath an "experimental" subcommand.

@hyangah
Copy link
Contributor Author

hyangah commented Dec 1, 2023

It looks like gopls execute deserves a separate discussion. Is there a reason to keep gopls.add_telemetry LSP command? Strictly speaking, the proxying capability doesn't need to be in gopls and it's not gopls's core functionality. We are currently adding this to gopls just because we don't want users to install another binary and gopls already has dependency on x/telemetry/counter.

@findleyr
Copy link
Contributor

findleyr commented Dec 1, 2023

Are there any counters that VS Code would want to update with high frequency? I.e., is there any performance advantage for an LSP command? I understand not now, but perhaps in the future?

As described, the advantage of an add_telemetry command is that it need not involve any session initialization.

I'm fine with both execute and add_telemetry. However, perhaps they both should be hidden from the help text, as neither are expected to be useful for users. Their presence is confusing.

@adonovan
Copy link
Member

adonovan commented Dec 5, 2023

The 'gopls execute' command now exists and could be used for this purpose, but in discussion with @hyangah today she was inclined towards a Go-based helper program in the vscode-go project that would include any features that need to be implemented in Go (such as telemetry) but have nothing to do with LSP or gopls. That also seems like reasonable solution.

@hyangah hyangah modified the milestones: Unreleased, gopls/backlog Dec 7, 2023
@hyangah hyangah closed this as not planned Won't fix, can't repro, duplicate, stale Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants