chore: improve java upgrade telemetry tracking and extension installation flow#1030
Open
frankliu20 wants to merge 6 commits into
Open
Conversation
frankliu20
requested review from
chagong,
testforstephen and
wenytang-ms
as code owners
frankliu20
force-pushed
the
haital-microsoft/fix-upgrade-notification-telemetry
branch
5 times, most recently
from
faf0be4 to
447c880
Compare
frankliu20
changed the title
frankliu20
force-pushed
the
haital-microsoft/fix-upgrade-notification-telemetry
branch
from
27162b6 to
5eeef91
Compare
Follow-up to v0.27.5 telemetry analysis (devdiv-azure-service-dmitryr/azure-java-migration-copilot-vscode-extension#5979): - Add a source field (SOURCE_CVE / SOURCE_JAVA_UPGRADE) to the upgradeNotification.show event so NOTIFY-level data can be split by CVE vs upgrade and aligned with downstream funnel stages. - Emit an installSucceeded event (with the initial extensionState, so fresh installs and updates can be told apart) right after installExtension resolves. - Drop the post-install getExtensionState re-check that produced false activation-timeout results; the not-installed path now reports proceeded and returns true after install completes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
frankliu20
force-pushed
the
haital-microsoft/fix-upgrade-notification-telemetry
branch
from
5eeef91 to
a3b367f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the Java upgrade notification → upgradeFlow telemetry funnel to improve attribution and reduce false “activation-timeout” outcomes observed in production telemetry.
Changes:
- Adds a NOTIFY-level
sourcedimension (SOURCE_CVEvsSOURCE_JAVA_UPGRADE) toupgradeNotification.showtelemetry. - Emits a new
upgradeFlow.installSucceededtelemetry event immediately afterinstallExtensionresolves. - Simplifies the
not-installedupgrade path to proceed after install (removing the post-installgetExtensionState()re-check).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/upgrade/utility.ts | Adds post-install telemetry and changes the post-install proceed/return behavior in the upgrade flow. |
| src/upgrade/display/notificationManager.ts | Adds a source dimension to the upgrade notification “show” telemetry event. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wenytang-ms
reviewed
Jun 9, 2026
Address review: avoid adding a new business operationName for the install step. Install success is already implied by the subsequent upgradeFlow.result event under the same operation ID. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review: instead of a dedicated installSucceeded operationName, reuse the stable java.dependency.upgradeFlow.result event and mark the install step with an upgradeFlowStep dimension (plus extensionState so fresh installs and updates can be told apart). Easier to query and evolve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Address review: extensionState was the pre-install value, so tagging a
'succeeded' event with 'not-installed'/'outdated' was contradictory.
Replace it with installType ('installed' vs 'updated') derived from the
pre-install state, which is unambiguous and avoids an unreliable
post-install state recompute (the original activation-race bug).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
extensions.getExtension() returns undefined both when the extension is disabled and when it is freshly installed but not yet activated, so the 'seems disabled, enable it manually' prompt is misleading right after a successful install. Skip it on the upgrade path; the function and the modernization caller are left unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the fixed 2s wait after a fresh install with an event-driven wait: resolve immediately if the extension is already registered, otherwise listen on extensions.onDidChange and proceed as soon as it appears, with a 5s timeout fallback. Returns as early as possible while still bounding the wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
frankliu20
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Production telemetry from
vscode-java-dependencyv0.27.5 (7d) surfaced gaps in theupgrade notification ->
upgradeFlowfunnel (follow-up to #1022, which already cut theoriginal "command not found" error rate from 23.2% to 0.5%):
upgradeNotification.showevent could not be split by CVE vs upgrade, soper-bucket click rates were diluted at the NOTIFY level.
not-installedflow starts were classified asactivation-timeout, but 0 ofthose carried an install error. The post-install check was the problem, not the install:
installExtensionresolves on install completion, while the code then immediatelyre-read
getExtensionState()and a fixedsetTimeout(2000)before the extension hadfinished registering, so a healthy install was frequently mislabeled as a timeout.
What changed
1. Split NOTIFY by CVE vs upgrade (
notificationManager.ts)Add a
sourcefield (SOURCE_CVE/SOURCE_JAVA_UPGRADE) to theupgradeNotification.showevent, reusing the same constants the click handler alreadypasses to the upgrade command so NOTIFY and downstream funnel stages align on one dimension.
2. Record install completion as a step on the existing result event (
utility.ts)Emit install success right after
installExtensionresolves, but without a new businessoperationName: it reuses the stablejava.dependency.upgradeFlow.resultevent with anupgradeFlowStep: "installSucceeded"dimension, plusinstallType("installed"for afresh install,
"updated"for an outdated one).installTypeis derived from thepre-install state, so it stays unambiguous and avoids an unreliable post-install state
recompute.
3. Replace the unreliable activation wait with event-driven readiness (
utility.ts)On the
not-installedpath, drop thegetExtensionState()re-check that produced the falseactivation-timeout. A newwaitForExtensionReady()helper resolves immediately if theextension is already registered, otherwise listens on
extensions.onDidChangeand proceedsas soon as it appears, with a 5s timeout fallback. The flow then reports
result: "proceeded"and returnstrue.The upgrade flow no longer calls
checkOrPromptToEnableAppModExtension():getExtension()returns
undefinedboth when an extension is disabled and when it is freshly installed butnot yet activated, so the "seems disabled, enable it manually" prompt was misleading right
after a successful install. The helper is unchanged and still used by the modernization flow.