Improve query insights static analysis and performance rating#615
Merged
Conversation
…ionViewRouter to use QueryObject
Co-authored-by: Copilot <copilot@github.com>
…eshold constants Task 1: Added 'diagnosticId' field to PerformanceDiagnostic interface in both ExplainPlanAnalyzer.ts and types/queryInsights.ts. All existing diagnostic pushes now include a stable snake_case diagnosticId for filtering/matching. Task 2: Added index strategy advisory threshold constants at the top of ExplainPlanAnalyzer.ts (COVERAGE_LOW_SELECTIVITY, COVERAGE_HIGH_RETURN, CARDINALITY_PER_KEY_RATIO, MULTIKEY_WARN_THRESHOLD, MULTIKEY_SEVERE_THRESHOLD).
… addIndexStrategyAdvisories Task 3: Added findStageInPlan() - generic recursive stage search in plan trees. Task 4: Added detectLowCardinalityIndex() - detects low-cardinality indexes via three signals: isBitmap flag, boolean filter literals, and estimatedEntryCount. Task 5: Added addIndexStrategyAdvisories() - appends neutral/negative diagnostics for coverage, cardinality, and multikey expansion. Severe multikey (≥20×) demotes score by one level.
Task 6: In collectionViewRouter.ts, fetch estimateDocumentCount() after execution stats, call addIndexStrategyAdvisories() before transform, and pass totalCollectionDocs to transformStage2Response(). Task 7: Updated transformStage2Response signature to accept totalCollectionDocs. Replaced efficiencyAnalysis.executionStrategy with selectivity (percentage of collection returned) and efficiencyAnalysis.examinedReturnedRatio with fetchOverhead (state-based label). Added computeSelectivity() and computeFetchOverhead() helpers. Updated createFailedQueryResponse() for the new efficiencyAnalysis shape. Removed unused formatRatioForDisplay().
Task 8: Updated QueryInsightsTab 2×2 grid:
- Replaced 'Execution Strategy' cell with 'Selectivity' (percentage of
collection returned, or '—' if unavailable)
- Replaced 'Examined-to-Returned Ratio' cell with 'Fetch Overhead'
(state-based label: Direct fetch, Covered query, Collection scan, etc.)
- Changed 'Index Used' null placeholder from 'None' to 'None (collection scan)'
Task 9: Updated PerformanceRatingCell badge rendering:
- Filter: Only show 3 positive badges (high_efficiency_ratio, fast_execution,
index_used). All neutral/negative badges always shown.
- Sort: Badges ordered positive → neutral → negative.
- Color: positive=success (green), neutral=informative (blue/gray),
negative=warning (orange/yellow). Previously negative used informative.
Task 10: Updated localization bundle with new strings (Selectivity, Fetch Overhead, Direct fetch, Covered query, Collection scan, Multikey expansion, No matches, None (collection scan), Query failed). Applied prettier formatting.
Added PERFORMANCE-RATING.md next to ExplainPlanAnalyzer.ts with: - End-user-friendly explanation of how the rating is computed - ASCII diagrams showing the data flow - Threshold tables for all scoring dimensions - Four worked examples (well-optimized, missing index, low cardinality, multikey expansion) - Glossary of terms Added implementation-notes.md documenting deviations from the approved implementation plan and all user-facing message changes.
…s scenario Co-authored-by: Copilot <copilot@github.com>
…y filter for improved diagnostics
… edge case fixes - Added tooltipExplanation support to CellBase/GenericCell (wraps entire cell) - All 4 efficiency analysis cells have dynamic tooltips based on current value - Fixed empty-query detection: pass user's actual parsed filter from session instead of unreliable command.filter extraction from explain result - Fixed zero-results edge case: show 'No matching documents' (neutral) instead of misleading 'Very low efficiency ratio' - Removed em dashes from user-facing strings - Formatted low-cardinality reasons with bullet points - l10n bundle updated
- Added InfoRegular icon to MetricBase labels when tooltipExplanation is present, matching the pattern used on efficiency analysis cells - Removed cursor:help from cellWithTooltip CSS class - Added inline-flex + align-items to dataHeader for icon alignment
…yling for metrics display Co-authored-by: Copilot <copilot@github.com>
…metrics and summary cells Co-authored-by: Copilot <copilot@github.com>
… indexes and improving efficiency checks Co-authored-by: Copilot <copilot@github.com>
…is.md Comprehensive reference covering: - Data flow from router to webview - All 4 summary indicators with possible values and tooltip logic - All diagnostic badges with IDs, conditions, types, and messages - Performance score decision tree - Index strategy advisories (coverage, cardinality, multikey) - Badge visibility filtering, ordering, and coloring - 5 documented design decisions with problem/fix reasoning: 1. Zero-results edge case 2. Empty query detection (DocumentDB command string) 3. Compound index cardinality false positives 4. Badge message formatting 5. Collection scan message accuracy
… analysis documentation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines the Query Insights Stage 2 “static” performance evaluation and its UI presentation, adds index-strategy advisories, and introduces a connector-side workaround for incorrect Azure DocumentDB explain metrics.
Changes:
- Reworked Stage 2 “Query Efficiency Analysis” cells to use Selectivity and Fetch Overhead, plus richer tooltips and badge filtering/sorting via new
diagnosticId. - Added index strategy advisories (coverage / low-cardinality / multikey expansion) to Stage 2 diagnostics and plumbed
estimateDocumentCount()for selectivity + advisory gating. - Added Azure DocumentDB explain fixup to correct
totalKeysExamined/keysExaminedfor COLLSCAN and covered it with unit tests.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/webviews/documentdb/collectionView/types/queryInsights.ts | Updates Stage 2 response shape (selectivity, fetchOverhead) and adds diagnosticId to diagnostics. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/queryInsights.scss | Adds shared tooltip/info-icon styling used by metrics and summary cells. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/summaryCard/custom/PerformanceRatingCell.tsx | Filters/sorts diagnostics using diagnosticId and adds negative badge coloring. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/summaryCard/SummaryCard.scss | Adds focus-visible styling for tooltip-wrapped cells. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/summaryCard/GenericCell.tsx | Plumbs optional tooltipExplanation down to CellBase. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/summaryCard/CellBase.tsx | Adds label tooltips + inline info icon for summary cells. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/metricsRow/MetricsRow.scss | Removes old metric tooltip styles (moved to shared tooltip classes). |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/components/metricsRow/MetricBase.tsx | Uses shared tooltip classes and adds info icon next to labels with tooltips. |
| src/webviews/documentdb/collectionView/components/queryInsightsTab/QueryInsightsTab.tsx | Replaces Stage 2 cells, adds dynamic tooltips, and wires new response fields. |
| src/webviews/documentdb/collectionView/collectionViewRouter.ts | Passes parsed filter to analysis, fetches collection count, and builds QueryObject for AI stage. |
| src/services/ai/QueryInsightsAIService.ts | Switches Stage 3 input from FindQueryParams to QueryObject and removes old conversion code. |
| src/documentdb/utils/fixupDocumentDbExplain.ts | Implements Azure DocumentDB explain workaround (zero out keys examined on COLLSCAN). |
| src/documentdb/utils/fixupDocumentDbExplain.test.ts | Adds test coverage for explain fixup across COLLSCAN, IXSCAN, sharded, and edge cases. |
| src/documentdb/queryInsights/transformations.ts | Computes selectivity and fetchOverhead and updates Stage 2 transformation accordingly. |
| src/documentdb/queryInsights/query-insights-static-analysis.md | Adds a comprehensive static-analysis reference doc for indicators, badges, and scoring. |
| src/documentdb/queryInsights/ExplainPlanAnalyzer.ts | Adds diagnosticId, index-strategy advisory logic, and updated diagnostic wording. |
| src/documentdb/ClustersClient.ts | Applies explain fixup to LLM enhanced explain APIs (find/aggregate/count). |
| src/documentdb/ClusterSession.ts | Applies explain fixup to Query Insights explain paths and updates Azure DocumentDB wording. |
| l10n/bundle.l10n.json | Updates localization bundle entries for new labels/strings (but see review comments re key mismatches). |
| docs/ai-and-plans/query-insights-implementation-plan.md | Adds an implementation plan document for the performance rating work. |
| docs/ai-and-plans/query-insights-implementation-notes.md | Adds implementation notes documenting deviations and final message set. |
Comments suppressed due to low confidence (1)
src/webviews/documentdb/collectionView/components/queryInsightsTab/components/summaryCard/custom/PerformanceRatingCell.tsx:160
key={index}on diagnostic badges is unstable when the list is filtered/sorted, which can cause React to reuse the wrong tooltip/badge when diagnostics change. Use a stable key such asdiagnostic.diagnosticId(or a composite including it) instead of the array index.
{visibleDiagnostics.map((diagnostic, index) => (
<Tooltip
key={index}
content={{
children: (
The low-cardinality advisory was reading queryFilter from explainResult.command.filter, which DocumentDB can return as a string. Now accepts an optional queryFilter parameter and passes the user's actual filter from the router, with a fallback to command.filter.
The boolean-filter signal in detectLowCardinalityIndex only checked
top-level values, missing operator forms like { $eq: true },
logical operators ($and/$or/$nor), and nested document shapes.
Added filterContainsBoolean() that recursively walks the filter tree
to detect boolean primitives at any depth.
The fetch overhead tooltip was branching on localized display text (e.g., checking for 'Covered', 'Collection scan' substrings), which breaks in non-English locales. Added a FetchOverheadKind type and return it alongside the localized label from computeFetchOverhead(). The UI now branches on the stable kind instead of the localized string.
The remarks block still referenced the removed examinedReturnedRatio display field and rating-specific concerns array. Updated to describe the current selectivity, fetchOverhead/fetchOverheadKind, and performanceRating.diagnostics fields.
When estimateDocumentCount() returns a stale count lower than nReturned, coverage/selectivity could exceed 100%. Now clamped to 1.0 (100%) in both addIndexStrategyAdvisories and computeSelectivity to prevent misleading display values.
Updated JSDoc to document that the input is mutated in place and the return value is the same reference. Removed the unnecessary '| undefined' from the type assertions in ClustersClient since fixupDocumentDbExplain only returns undefined when its input is undefined (which cannot happen in these call sites).
Wrapped all user-facing diagnostic messages, details, and reason strings in ExplainPlanAnalyzer with l10n.t() calls. This includes performance rating diagnostics, execution error messages, low-cardinality detection reasons, and index strategy advisory messages. Regenerated the l10n bundle to fix the CI gate failure.
Collaborator
Author
Review fixes appliedAddressed review findings from
Deferred per operator instruction:
|
Added 'npx jest --no-coverage' as step 4 in the PR Completion Checklist to ensure agents verify all tests pass before finishing.
Three build errors existed because lint/tests use different tsconfig: 1. Sharded path used old computeFetchOverhead return type (string vs object) 2. Failed-query response missing fetchOverheadKind property 3. queryParams referenced outside its block scope — hoisted filter Also added 'npm run build' as step 5 in the PR Completion Checklist.
Contributor
✅ Code Quality Checks
This comment is updated automatically on each push. |
Contributor
📦 Build Size Report
Download artifact · updated automatically on each push. |
bk201-
approved these changes
Apr 27, 2026
github-project-automation
Bot
moved this to Done
in DocumentDB for VSCode: Release Plan
Merged
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.
Summary
Improves the Query Insights static query performance evaluation based on user feedback gathered during testing. Also includes infrastructure fixes for filter/project/sort object handling and a workaround for Azure DocumentDB explain results.
Changes
Infrastructure Fixes
QueryInsightsAIServiceandcollectionViewRouterto use the newQueryObjectconversion, passing the user's actual parsed filter toanalyzeExecutionStats()instead of extracting from the unreliablecommand.filterin the explain result (DocumentDB returnscommandas a string, not a document).fixupDocumentDbExplain()as a temporary workaround for Azure DocumentDB reportingtotalKeysExaminedvalues in explain results when no keys were actually examined. Includes test coverage.Static Query Performance Evaluation Improvements
New features:
diagnosticIdto allPerformanceDiagnosticobjects for stable filtering/matchingexecutionStrategy/examinedReturnedRatiocells withSelectivity(% of collection returned) andFetch Overhead(state-based label: Direct fetch, Covered query, Collection scan, etc.)Edge case fixes based on user feedback:
Styling:
Documentation
query-insights-static-analysis.md: comprehensive reference covering all 4 indicators, all badges, the score decision tree, and 5 documented design decisions with reasoningTesting
fixupDocumentDbExplain