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

refactor(logging): scope log messages to specific LSP using topic headers. #6526

Open
wants to merge 7 commits into
base: feature/amazonqLSP
Choose a base branch
from

Conversation

Hweinstock
Copy link
Contributor

Problem

The log messages coming out of the LSP work do not make it clear which LSP its working with (Q Workspace or Q general).
Follow up from: #6385 (comment)

Solution

  • Use new LogTopic functionality to scope these to specific LSP.
  • Add more logging information from LSP cleanup process.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Hweinstock Hweinstock marked this pull request as ready for review February 7, 2025 16:15
@Hweinstock Hweinstock requested review from a team as code owners February 7, 2025 16:15
| 'test'
| 'childProcess'
| 'lsp'
| 'QWorkspaceLsp'
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be amazonqWorkspaceLSP or is that too long? I feel like we need to have conventions on whether we use q vs amazonq since both are used in the codebase 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That feels more consistent. Better to start verbose and look for opportunities to shorten later.

} from 'aws-core-vscode/shared'
import path from 'path'

const manifestURL = 'https://aws-toolkit-language-servers.amazonaws.com/codewhisperer/0/manifest.json'
export const supportedLspServerVersions = '^2.3.0'

const logger = getLogger('amazonqLSP')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const logger = getLogger('amazonqLSP')
const logger = getLogger('amazonqLsp')


const manifestUrl = 'https://aws-toolkit-language-servers.amazonaws.com/q-context/manifest.json'
// this LSP client in Q extension is only going to work with these LSP server versions
const supportedLspServerVersions = '0.1.35'
const logger = getLogger('amazonqWorkspaceLSP')
Copy link
Contributor

Choose a reason for hiding this comment

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

amazonqWorkspaceIndexingLsp is too long, but probably "indexing" is the most important thing to call out?

Suggested change
const logger = getLogger('amazonqWorkspaceLSP')
const logger = getLogger('amazonqIndexingLsp')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now, indexing is the only action supported by this lsp, but I could imagine Q adding a feature that does other "work" within the workspace. Also, I think it might be best to keep it inline with the name field we use in the resolver here:

Perhaps we should pull these out somewhere to make them more discoverable, but that might involve a deeper refactor out of the scope here.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

Great idea

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