Skip to content

fix(lmtool): stop double-counting classNameDetection on every invoke#1651

Open
wenytang-ms wants to merge 2 commits into
mainfrom
fix/lmtool-dedup-classname-detection
Open

fix(lmtool): stop double-counting classNameDetection on every invoke#1651
wenytang-ms wants to merge 2 commits into
mainfrom
fix/lmtool-dedup-classname-detection

Conversation

@wenytang-ms

@wenytang-ms wenytang-ms commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Update (review pass): the original PR description claimed this dedup would deflate the observed noPackage failure rate "from 49% to ~25%". That claim is wrong — the duplicate findFullyQualifiedClassName call in debugJavaApplication only ran the FS walk a second time; recordLaunchInternal({name:'classNameDetection'}) was only emitted from constructDebugCommand. So:

Problem

debugJavaApplication (the debug_java_application LMT handler) was calling
findFullyQualifiedClassName twice per invocation for the simple-class-name path:

  1. Once inside constructDebugCommand (to resolve the FQN for the actual
    java -cp ... ClassName command), where it also emitted
    recordLaunchInternal({name: 'classNameDetection', detected: <bool>}).
  2. Once in the caller, immediately after constructDebugCommand, just to
    build the user-facing targetInfo string ("com.example.App (detected from App)").

findFullyQualifiedClassName walks src/main/java (or equivalent) recursively
up to MAX_FILE_SEARCH_DEPTH, stats every .java file, and reads candidates
to extract the package. Doing it twice on every launch is wasted work on the
hot path.

Fix

  • Move the detection call (and the classNameDetection telemetry emit) into
    the caller (debugJavaApplication, Step 3), so it runs exactly once per
    invocation.
  • Add an optional third parameter preDetectedClassName?: string | null to
    constructDebugCommand. When the caller has already resolved the FQN, it
    passes that result in and constructDebugCommand skips re-detection.
  • Reuse the same resolved name in the targetInfo formatting block — no
    second FS walk.

Scope

  • No behavior change for users. The resolved FQN, the constructed
    debugjava command, and the telemetry event content are identical.
  • No telemetry-count changeclassNameDetection still fires exactly
    once per invocation (it always did; the second call site was not emitting).
  • One extra FS walk eliminated per simple-class-name launch.

Test

  • npx tsc --noEmit — clean
  • npm run tslint — clean
  • Diff: +44 / -27 in 1 file (after review reword)

Validation plan

This PR is a refactor, so there is no metric we expect to move. Post-ship
sanity check: per-invocation classNameDetection event volume should stay
flat (1:1 with debug_java_application.invoke). If it drops by ~50%, that
would actually confirm a pre-existing 2× emission elsewhere we missed; if it
stays equal, the refactor is a pure perf win, which matches the code reading.

RawEventsVSCodeExt
| where ServerTimestamp > ago(14d)
| where ExtensionName == "vscjava.vscode-java-debug"
| extend op = tostring(Properties["operationname"])
| where op endswith "debug_java_application.invoke"
   or op endswith "classNameDetection"
| summarize count() by op, extversion = tostring(Properties["common.extversion"])
| order by extversion desc, op

Related

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

findFullyQualifiedClassName was called twice for every invocation that used a simple class name:

  1. inside constructDebugCommand to resolve the actual command

  2. immediately after, to build the targetInfo message shown to the model

Each call also emitted its own classNameDetection telemetry event, so the classNameDetection success/failure counts in ddtelfiltered were inflated by 2x.

This makes the apparent ~49% noPackage failure rate look much worse than reality (the true rate is closer to ~25% before accounting for other root causes addressed in #1650).

Fix: resolve the detection once at the caller, emit telemetry once, and pass the resolved name into both constructDebugCommand and the targetInfo branch.

Side benefit: removes a redundant file system walk from the hot launch path.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors debugJavaApplication to resolve a simple Java class name to a fully-qualified class name once per invocation and reuse that result for both command construction and the user-facing targetInfo message.

Changes:

  • Adds a single, caller-owned findFullyQualifiedClassName lookup and reuses its result across the launch flow.
  • Updates constructDebugCommand to accept an optional preDetectedClassName to avoid redundant lookups and centralize telemetry emission at the caller.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/languageModelTool.ts Outdated
Comment thread src/languageModelTool.ts Outdated
Reviewer correctly pointed out that the original main branch only had a duplicate findFullyQualifiedClassName FS walk in the targetInfo branch; the recordLaunchInternal({name:'classNameDetection'}) emission was only inside constructDebugCommand, not duplicated. The 'inflates noPackage by 2x' rationale was wrong — observed metrics are not inflated by the dup.

Reworded both comments to focus on the FS-walk dedup + ownership clarity. Functional code unchanged; only comments updated.
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.

2 participants