Skip to content

Conversation

rli
Copy link
Contributor

@rli rli commented Jun 4, 2025

We need some visibility on how prevalent failures are

  • Manifest fetch
  • Download artifact
  • Validate artifact
  • end result is bundled or from flare
  • is nodejs from flare or local

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

We need some visibility on how prevalent failures are
@rli rli requested a review from a team as a code owner June 4, 2025 22:14
Copy link

github-actions bot commented Jun 4, 2025

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at [email protected]

if (!nodeRuntime.isNullOrEmpty()) {
LOG.info { "Using node from $nodeRuntime " }

resolveNodeMetric(false, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is it possible to add parameter names?

if (!validateFileHash(filePath, expectedHash)) {
it.success(false)

val exception = LspException("Hash mismatch after re-download for ${filePath.fileName}", LspException.ErrorCode.HASH_MISMATCH)
Copy link
Contributor

@manodnyab manodnyab Jun 4, 2025

Choose a reason for hiding this comment

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

Should we run this through the util that removes username(scrubNames)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's path.fileName so should not have username because this is just the server version

val resolveNodeMetric = { isBundled: Boolean, success: Boolean ->
Telemetry.languageserver.setup.use {
it.metadata("languageServerSetupStage", "resolveNode")
it.metadata("credentialStartUrl", getStartUrl(project))
Copy link
Contributor

Choose a reason for hiding this comment

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

getStartUrl may be null if user has not login but I think it's fine

Comment on lines 205 to 211
if (!filePath.exists()) {
logger.info { "Downloading file: ${filePath.fileName}" }
saveFileFromUrl(url, filePath, ProgressManager.getInstance().progressIndicator)
recordDownload { saveFileFromUrl(url, filePath, ProgressManager.getInstance().progressIndicator) }
}
if (!validateFileHash(filePath, expectedHash)) {
logger.warn { "Hash mismatch for ${filePath.fileName}, re-downloading" }
filePath.deleteIfExists()
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to this PR but it seems like we can download twice if the first downloaded one has hash mismatch due to antivirus changing content, but still acceptable

@rli rli merged commit bc95823 into feature/q-lsp-chat Jun 5, 2025
14 of 16 checks passed
@rli rli deleted the rli/flare-fetch-metrics branch June 5, 2025 04:27
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