-
Notifications
You must be signed in to change notification settings - Fork 272
fix(amazonq): add metrics for flare/node resolution #5786
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,10 +77,12 @@ import software.aws.toolkits.jetbrains.services.amazonq.lsp.textdocument.TextDoc | |
import software.aws.toolkits.jetbrains.services.amazonq.lsp.util.WorkspaceFolderUtil.createWorkspaceFolders | ||
import software.aws.toolkits.jetbrains.services.amazonq.lsp.workspace.WorkspaceServiceHandler | ||
import software.aws.toolkits.jetbrains.services.amazonq.profile.QDefaultServiceConfig | ||
import software.aws.toolkits.jetbrains.services.cwc.controller.chat.telemetry.getStartUrl | ||
import software.aws.toolkits.jetbrains.services.telemetry.ClientMetadata | ||
import software.aws.toolkits.jetbrains.settings.LspSettings | ||
import software.aws.toolkits.jetbrains.utils.notifyInfo | ||
import software.aws.toolkits.resources.message | ||
import software.aws.toolkits.telemetry.Telemetry | ||
import java.io.IOException | ||
import java.io.OutputStreamWriter | ||
import java.io.PipedInputStream | ||
|
@@ -526,20 +528,35 @@ private class AmazonQServerInstance(private val project: Project, private val cs | |
* may fail to start in that case. The caller should handle potential runtime initialization failures. | ||
*/ | ||
private fun getNodeRuntimePath(nodePath: Path): Path { | ||
val resolveNodeMetric = { isBundled: Boolean, success: Boolean -> | ||
Telemetry.languageserver.setup.use { | ||
it.metadata("languageServerSetupStage", "resolveNode") | ||
it.metadata("credentialStartUrl", getStartUrl(project)) | ||
it.setAttribute("isBundledNode", isBundled) | ||
it.success(success) | ||
} | ||
} | ||
|
||
if (Files.exists(nodePath) && Files.isExecutable(nodePath)) { | ||
resolveNodeMetric(true, true) | ||
return nodePath | ||
} | ||
|
||
// use alternative node runtime if it is not found | ||
LOG.warn { "Node Runtime download failed. Fallback to user specified node runtime " } | ||
// attempt to use user provided node runtime path | ||
val nodeRuntime = LspSettings.getInstance().getNodeRuntimePath() | ||
if (!nodeRuntime.isNullOrEmpty()) { | ||
LOG.info { "Using node from $nodeRuntime " } | ||
|
||
resolveNodeMetric(false, true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: is it possible to add parameter names? |
||
return Path.of(nodeRuntime) | ||
} else { | ||
val localNode = locateNodeCommand() | ||
if (localNode != null) { | ||
LOG.info { "Using node from ${localNode.toAbsolutePath()}" } | ||
|
||
resolveNodeMetric(false, true) | ||
return localNode | ||
} | ||
notifyInfo( | ||
|
@@ -557,6 +574,8 @@ private class AmazonQServerInstance(private val project: Project, private val cs | |
) { _, notification -> notification.expire() } | ||
) | ||
) | ||
|
||
resolveNodeMetric(false, false) | ||
return nodePath | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,10 @@ import software.aws.toolkits.core.utils.getLogger | |
import software.aws.toolkits.core.utils.info | ||
import software.aws.toolkits.core.utils.warn | ||
import software.aws.toolkits.jetbrains.core.saveFileFromUrl | ||
import software.aws.toolkits.jetbrains.services.cwc.controller.chat.telemetry.getStartUrl | ||
import software.aws.toolkits.resources.AwsCoreBundle | ||
import software.aws.toolkits.telemetry.LanguageServerSetupStage | ||
import software.aws.toolkits.telemetry.Telemetry | ||
import java.nio.file.Files | ||
import java.nio.file.Path | ||
import java.nio.file.Paths | ||
|
@@ -106,33 +109,33 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH, | |
} | ||
|
||
suspend fun tryDownloadLspArtifacts(project: Project, targetVersion: Version, target: VersionTarget): Path? { | ||
val temporaryDownloadPath = Files.createTempDirectory("lsp-dl") | ||
val downloadPath = lspArtifactsPath.resolve(targetVersion.serverVersion.toString()) | ||
val destinationPath = lspArtifactsPath.resolve(targetVersion.serverVersion.toString()) | ||
|
||
while (currentAttempt.get() < maxDownloadAttempts) { | ||
currentAttempt.incrementAndGet() | ||
logger.info { "Attempt ${currentAttempt.get()} of $maxDownloadAttempts to download LSP artifacts" } | ||
val temporaryDownloadPath = Files.createTempDirectory("lsp-dl") | ||
|
||
try { | ||
return withBackgroundProgress( | ||
project, | ||
AwsCoreBundle.message("amazonqFeatureDev.placeholder.downloading_and_extracting_lsp_artifacts"), | ||
cancellable = true | ||
) { | ||
if (downloadLspArtifacts(temporaryDownloadPath, target) && !target.contents.isNullOrEmpty()) { | ||
moveFilesFromSourceToDestination(temporaryDownloadPath, downloadPath) | ||
if (downloadLspArtifacts(project, temporaryDownloadPath, target) && !target.contents.isNullOrEmpty()) { | ||
moveFilesFromSourceToDestination(temporaryDownloadPath, destinationPath) | ||
target.contents | ||
.mapNotNull { it.filename } | ||
.forEach { filename -> extractZipFile(downloadPath.resolve(filename), downloadPath) } | ||
logger.info { "Successfully downloaded and moved LSP artifacts to $downloadPath" } | ||
.forEach { filename -> extractZipFile(destinationPath.resolve(filename), destinationPath) } | ||
logger.info { "Successfully downloaded and moved LSP artifacts to $destinationPath" } | ||
|
||
val thirdPartyLicenses = targetVersion.thirdPartyLicenses | ||
logger.info { | ||
"Installing Amazon Q Language Server v${targetVersion.serverVersion} to: $downloadPath. " + | ||
"Installing Amazon Q Language Server v${targetVersion.serverVersion} to: $destinationPath. " + | ||
if (thirdPartyLicenses == null) "" else "Attribution notice can be found at $thirdPartyLicenses" | ||
} | ||
|
||
return@withBackgroundProgress downloadPath | ||
return@withBackgroundProgress destinationPath | ||
} | ||
|
||
return@withBackgroundProgress null | ||
|
@@ -146,15 +149,15 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH, | |
else -> { logger.error(e) { "Failed to download/move LSP artifacts on attempt ${currentAttempt.get()}" } } | ||
} | ||
temporaryDownloadPath.toFile().deleteRecursively() | ||
downloadPath.toFile().deleteRecursively() | ||
destinationPath.toFile().deleteRecursively() | ||
} | ||
} | ||
logger.error { "Failed to download LSP artifacts after $maxDownloadAttempts attempts" } | ||
return null | ||
} | ||
|
||
@VisibleForTesting | ||
internal fun downloadLspArtifacts(downloadPath: Path, target: VersionTarget?): Boolean { | ||
internal fun downloadLspArtifacts(project: Project, downloadPath: Path, target: VersionTarget?): Boolean { | ||
if (target == null || target.contents.isNullOrEmpty()) { | ||
logger.warn { "No target contents available for download" } | ||
return false | ||
|
@@ -171,7 +174,7 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH, | |
logger.warn { "No hash available for ${content.filename}" } | ||
return@forEach | ||
} | ||
downloadAndValidateFile(content.url, filePath, contentHash) | ||
downloadAndValidateFile(project, content.url, filePath, contentHash) | ||
} | ||
validateDownloadedFiles(downloadPath, target.contents) | ||
} catch (e: Exception) { | ||
|
@@ -182,18 +185,44 @@ class ArtifactHelper(private val lspArtifactsPath: Path = DEFAULT_ARTIFACT_PATH, | |
return true | ||
} | ||
|
||
private fun downloadAndValidateFile(url: String, filePath: Path, expectedHash: String) { | ||
private fun downloadAndValidateFile(project: Project, url: String, filePath: Path, expectedHash: String) { | ||
val recordDownload = { runnable: () -> Unit -> | ||
Telemetry.languageserver.setup.use { telemetry -> | ||
telemetry.languageServerSetupStage(LanguageServerSetupStage.GetServer) | ||
telemetry.metadata("credentialStartUrl", getStartUrl(project)) | ||
telemetry.success(true) | ||
|
||
try { | ||
runnable() | ||
} catch (t: Throwable) { | ||
telemetry.success(false) | ||
telemetry.recordException(t) | ||
} | ||
} | ||
} | ||
|
||
try { | ||
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() | ||
Comment on lines
206
to
212
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
saveFileFromUrl(url, filePath) | ||
if (!validateFileHash(filePath, expectedHash)) { | ||
throw LspException("Hash mismatch after re-download for ${filePath.fileName}", LspException.ErrorCode.HASH_MISMATCH) | ||
recordDownload { saveFileFromUrl(url, filePath) } | ||
|
||
Telemetry.languageserver.setup.use { | ||
it.languageServerSetupStage(LanguageServerSetupStage.Validate) | ||
it.metadata("credentialStartUrl", getStartUrl(project)) | ||
it.success(true) | ||
|
||
if (!validateFileHash(filePath, expectedHash)) { | ||
it.success(false) | ||
|
||
val exception = LspException("Hash mismatch after re-download for ${filePath.fileName}", LspException.ErrorCode.HASH_MISMATCH) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we run this through the util that removes username(scrubNames) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's |
||
it.recordException(exception) | ||
throw exception | ||
} | ||
} | ||
} | ||
} catch (e: Exception) { | ||
|
There was a problem hiding this comment.
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