Dataset UI and observability workspace + Trace automation #11096
Conversation
Signed-off-by: Adam Tackett <[email protected]>
📝 WalkthroughWalkthroughComprehensive refactor introducing a unified index selector component to replace legacy selector UI elements. Adds automated trace data detection and dataset creation utilities. Enhances dataset selection with signal-type compatibility checks and improved UI rendering. Updates workspace and plugin configurations to support observability workspace gating and trace auto-detection during workspace creation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DatasetSelect
participant UnifiedIndexSelector
participant IndexFetcher
participant IndexPatternService
participant SavedObjects
User->>DatasetSelect: Select dataset
Note over DatasetSelect: Check signal type compatibility
DatasetSelect->>DatasetSelect: Fetch available datasets
DatasetSelect->>DatasetSelect: Deduplicate datasets
DatasetSelect->>DatasetSelect: Filter by signalType
DatasetSelect->>User: Display compatible datasets
User->>DatasetSelect: Click "View datasets" or "Create dataset"
DatasetSelect->>DatasetSelect: Open ViewDatasetsModal
DatasetSelect->>User: Render dataset list with modal
User->>DatasetSelect: Select dataset or create new
alt Dataset Selected
DatasetSelect->>IndexPatternService: Validate selection
IndexPatternService-->>DatasetSelect: OK
DatasetSelect->>User: Close modal, update selection
else Create New Dataset
DatasetSelect->>UnifiedIndexSelector: Open selector
User->>UnifiedIndexSelector: Search/select indices
UnifiedIndexSelector->>IndexFetcher: Fetch matching indices
IndexFetcher->>IndexPatternService: Query with pattern
IndexPatternService-->>IndexFetcher: Results
IndexFetcher-->>UnifiedIndexSelector: Display results
User->>UnifiedIndexSelector: Add index or wildcard
UnifiedIndexSelector-->>DatasetSelect: Return selection
DatasetSelect->>SavedObjects: Create index-pattern
SavedObjects-->>DatasetSelect: Dataset created
DatasetSelect->>User: Close modal, use new dataset
end
sequenceDiagram
participant WorkspaceCreator
participant TraceDetection
participant IndexPatternService
participant SavedObjects
participant DatasetCreation
participant User as User
WorkspaceCreator->>TraceDetection: detectTraceDataAcrossDataSources()
loop For each data source
TraceDetection->>IndexPatternService: Query otel-v1-apm-span* indices
IndexPatternService-->>TraceDetection: Matching indices
TraceDetection->>IndexPatternService: Validate required fields
IndexPatternService-->>TraceDetection: Field validation result
end
TraceDetection-->>WorkspaceCreator: Detection results (traces/logs)
WorkspaceCreator->>DatasetCreation: createAutoDetectedDatasets()
DatasetCreation->>SavedObjects: Search/create trace dataset
SavedObjects-->>DatasetCreation: Dataset ID
DatasetCreation->>SavedObjects: Search/create log dataset
SavedObjects-->>DatasetCreation: Dataset ID
alt Both datasets exist
DatasetCreation->>SavedObjects: Create correlation
SavedObjects-->>DatasetCreation: Correlation ID
end
DatasetCreation-->>WorkspaceCreator: Creation results
WorkspaceCreator->>User: Show success toast & reload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
ℹ️ Manual Changeset Creation ReminderPlease ensure manual commit for changeset file 11096.yml under folder changelogs/fragments to complete this PR. If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link. For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool. |
Signed-off-by: Adam Tackett <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11096 +/- ##
==========================================
- Coverage 60.46% 60.32% -0.14%
==========================================
Files 4610 4617 +7
Lines 126664 127278 +614
Branches 21218 21389 +171
==========================================
+ Hits 76584 76785 +201
- Misses 44650 45030 +380
- Partials 5430 5463 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
TackAdam
requested review from
AMoo-Miki,
Flyingliuhub,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
curq,
joshuarrrr,
kavilla,
manasvinibs,
zengyan-amazon and
zhongnansu
as code owners
TackAdam
changed the title
|
Didn't look into the code yet, but in the screen record, I don't see schema mappings UI in the dataset creation UI, is this expected? |
| selectedIndexIds, | ||
| }: any) => ( | ||
| // Mock the UnifiedIndexSelector component | ||
| jest.mock('./unified_index_selector', () => ({ |
There was a problem hiding this comment.
I see there are changes in the data plugin. Have you validated that there are no regressions in the:
- non
query-enhancedexperience? (our oldest experience) query-enhancedexperience? (explore enabled, but non-obs workspace)
ps48
left a comment
ps48
left a comment
There was a problem hiding this comment.
Can we also check the below edge cases:
- what happens to the dataset creation automation when user has the permission to read the datasource but doesn't have write access?
- How are we sanitizing user input on wild cards in dataset creation?
- Does the cached health data have a TTL and is it per-datasource? Also, what is the user's FGAC permissions doesn't allow cat indices. What happens then?
...integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js
Outdated
Show resolved
Hide resolved
| flex-shrink: 0; | ||
| } | ||
|
|
||
| &__healthPopover { |
There was a problem hiding this comment.
are these particular to data structure creator? if not should add more specificity in css?
.indexDataStructureCreator {
&__unifiedSelector__healthPopover { ... }
&__unifiedSelector__wildcardPopover { ... }
}
src/plugins/explore/public/components/trace_auto_detect_callout.tsx
Outdated
Show resolved
Hide resolved
...ation/core_opensearch_dashboards/opensearch_dashboards/apps/explore/02/saved_queries.spec.js
Outdated
Show resolved
Hide resolved
...ery_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx
Outdated
Show resolved
Hide resolved
...ery_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx
Outdated
Show resolved
Hide resolved
...ry/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.scss
Show resolved
Hide resolved
...ery/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cypress/utils/commands.explore.js (2)
288-290: Toast timeout increased to 30 seconds.The timeout for toast notifications has been increased from the default to 30 seconds in both
saveQueryandupdateSavedQuerycommands. While this may improve stability for slow environments, consider documenting why such a long timeout is necessary.Also applies to: 328-330
501-501: Comment assumes UI behavior without verification.The comment states "Indexes panel is now hidden when it's the only option" but the code doesn't verify this assumption. If this assumption is wrong, the test could silently pass when it should fail.
Consider adding an assertion or explicit check to document and verify the UI state rather than relying on an unverified comment.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cypress/utils/apps/query_enhancements/commands.jscypress/utils/commands.explore.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (68)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
- GitHub Check: linkchecker
- GitHub Check: lighthouse
🔇 Additional comments (11)
cypress/utils/apps/query_enhancements/commands.js (4)
160-165: Conditional element handling looks reasonable.The defensive check for the "Indexes" element before clicking improves robustness when the UI conditionally shows/hides this option. The pattern correctly uses
cy.get('body').then()to check for element existence before attempting interaction.
169-185: Unified selector integration is well-structured.The migration to the unified index selector components follows a clear pattern:
- Type into the search field
- Wait for dropdown visibility
- Select from the results list
The use of
{ force: true }on clicks is acceptable in Cypress tests to avoid flakiness from transient overlays.
219-222: Good addition for test stability.Adding an explicit wait for
.euiSelectableListvisibility before interacting with dataset titles reduces flakiness from race conditions during list rendering.
248-254: Conditional handling mirrors the Indexes pattern appropriately.The defensive check for "Index Patterns" follows the same safe pattern used for "Indexes" (lines 160-165), maintaining consistency across the command file.
cypress/utils/commands.explore.js (7)
338-342: Good defensive checks added for button readiness.Adding explicit visibility and disabled state checks before clicking the
saved-query-management-open-buttonprevents race conditions when the popover is still rendering. This improves test reliability.Also applies to: 360-364
417-433: Unified selector integration follows the established pattern.The interaction flow correctly mirrors the pattern used in
query_enhancements/commands.js:
- Type into unified-index-selector-search
- Wait for unified-index-selector-dropdown
- Select from unified-index-selector-list
508-508: Consistent usage of the refactored function signature.The call to
selectIndexWildcardModewith a single parameter is consistent with the updated function signature at line 62.
661-668: Consistent pattern for wildcard selection.The code correctly skips the deprecated Indexes panel logic and proceeds directly to data source selection followed by wildcard mode selection using the unified approach.
745-748: Proper interception handling for dataset creation.The wait for
@createDatasetInterceptionfollowed by extracting and aliasing the created ID is a standard Cypress pattern for capturing created resource identifiers for use in later test steps.
62-78: This is not a breaking change — the function is only used internally.The
selectIndexWildcardModefunction is called only twice in the codebase, both at lines 508 and 668 within the same file, and both calls already use the 1-parameter signature. There are no external callers. The removal of theappendWildcardparameter is safe and reflects the UI now handling wildcard auto-appending internally (as noted in the function comment).Likely an incorrect or invalid review comment.
414-414: These are two distinct test IDs for different UI elements, not a typo. The sequence at lines 410–414 shows intentional usage: first clickingdatasetSelectButton(the main selector), thendatasetSelectorAdvancedButton(the advanced option). Both IDs are correctly defined insrc/plugins/data/public/ui/dataset_select/dataset_select.tsxand used consistently throughout the codebase.Likely an incorrect or invalid review comment.
|
cannot-remove.mov
|
The video above shows the Traces dataset creation without schema mappings section, however the Logs dataset creation flow does have the schema mappings section. Is that intended? Would there be a case users will need to change schema mapping for Traces datasets? |
The log page creation flow has the schema mappings: We do not have schema mapping for traces yet, or a use-case till we add jaeger support later on. |
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx:
- Line 267: The comment in handleSelectFromDropdown claims "Keep popover open to
allow multiple selections" but the code never re-opens the EuiPopover; either
implement that intent or remove the comment. Update handleSelectFromDropdown
(the function invoking onSelectionChange) to call setIsPopoverOpen(true) after
onSelectionChange resolves/returns to keep the popover open for additional
selections, ensuring you preserve any existing event handling and prevent
default closing behavior, or delete the misleading comment if you prefer the
default auto-closing behavior of EuiPopover.
In
@src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx:
- Around line 181-208: The empty catch blocks swallow useful errors during
per-data-source detection/dataset creation; update the catch inside the for-loop
(and the outer catch) to log a concise warning including the dataSourceId and
the error (e.g., use the existing logger in scope or console.warn with
error.stack/message) so failures are visible; optionally accumulate
per-data-source failures into an array and emit a single debug/warn summary in
the outer catch after the loop.
- Around line 155-227: The code mutates global savedObjects.client workspace via
savedObjects.client.setCurrentWorkspace(newWorkspaceId) without restoring it;
wrap the trace-detection block in a try-finally that saves
savedObjects.client.currentWorkspaceId to a local variable, calls
setCurrentWorkspace(newWorkspaceId), runs the detection/create logic, and in
finally restores the previous id using setCurrentWorkspace(previousWorkspaceId)
(or use a scoped SavedObjectsClient API if available) so subsequent saved-object
operations don't wrongly target the new workspace.
🧹 Nitpick comments (8)
src/plugins/dataset_management/public/plugin.ts (2)
98-121: Workspace access control logic is correct.The mount-time check properly gates access to observability workspaces only. Minor observation: the
?? falseon line 113 is redundant since the left operand already evaluates tofalsewhenfeaturesis falsy (due to the&&short-circuit).♻️ Optional simplification
const isObservabilityWorkspace = - (features && isNavGroupInFeatureConfigs(DEFAULT_NAV_GROUPS.observability.id, features)) ?? - false; + !!features && isNavGroupInFeatureConfigs(DEFAULT_NAV_GROUPS.observability.id, features);
172-196: Potential race condition in subscription lifecycle.If
stop()is called before thegetStartServices()promise resolves, the subscription created afterward won't be cleaned up, leading to a potential memory leak. This is unlikely in normal plugin lifecycle but could occur during rapid navigation or error scenarios.♻️ Optional defensive pattern
+ private stopped = false; private workspaceSubscription?: Subscription; // In setup(): core.getStartServices().then(([coreStart]) => { + if (this.stopped) return; if (coreStart.application.capabilities.workspaces.enabled) { this.workspaceSubscription = coreStart.workspaces.currentWorkspace$.subscribe( (workspace) => { // ... } ); } }); // In stop(): public stop() { + this.stopped = true; if (this.workspaceSubscription) {src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (1)
28-66: Make thedataservice typing match actual optionality (dataPlugin?.dataViews).You typed
data: DataPublicPluginStartas required (Line 65) but use it as optional (Line 157). Either makedataoptional in the hook typing or remove the optional chaining so failures are surfaced earlier.Proposed adjustment
} = useOpenSearchDashboards<{ workspaceClient: WorkspaceClient; dataSourceManagement?: DataSourceManagementPluginSetup; navigationUI: NavigationPublicPluginStart['ui']; useCaseService: UseCaseService; - data: DataPublicPluginStart; + data?: DataPublicPluginStart; }>();src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (3)
74-139: Consider adding request tracking to prevent race conditions.Unlike
useIndexFetcher(which usesrequestIdRef),fetchBatchIndexInfodoesn't track request IDs. If users rapidly change selections, stale responses could overwrite fresh data in the cache. Consider implementing request tracking similar to the pattern inuse_index_fetcher.ts.♻️ Suggested approach for request tracking
Add a ref to track requests:
+ const batchRequestIdRef = useRef<number>(0); + const fetchBatchIndexInfo = useCallback( async (indexNames: string[]) => { if (!services?.http || indexNames.length === 0) return; + batchRequestIdRef.current += 1; + const currentRequestId = batchRequestIdRef.current; + // Mark all indices as loading setLoadingInfoForIndexSet((prev) => {Then check before updating state:
const response = await services.http.post<any>(`/api/console/proxy`, { query: queryParams, body: '', }); + if (currentRequestId !== batchRequestIdRef.current) { + return; + } + if (response && Array.isArray(response)) {
304-513: Consider extracting sub-components from this 200+ line function.The
renderHealthPopoverfunction handles two distinct UI patterns (wildcard matches and health info) and spans over 200 lines. Consider extractingWildcardMatchPopoverandHealthInfoPopoveras separate components to improve readability and testability.
89-91: HTTP POST is already being used for transport; consider adding explicit batch size limits as a defensive measure.The path parameter does concatenate all index names without explicit chunking (lines 89-91), which could theoretically exceed path length limits. However, the request is already sent via HTTP POST to
/api/console/proxyrather than as a GET request, providing much higher practical limits. Additionally, the current pagination and fetch patterns naturally limit requests:
- Wildcard indices are paginated with
pageSize: 10(line 204), so batch fetches for visible items are capped at ~10 indices- Exact indices are fetched individually (line 184)
To make this more robust, consider adding an explicit batch size constant and chunking the
indexNamesarray infetchBatchIndexInfobefore construction, even though current usage patterns make it unlikely to be triggered.src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx (2)
204-214: Auto-wildcard behavior may surprise users.Automatically appending a wildcard when typing a single alphanumeric character is a helpful UX optimization, but users unfamiliar with this behavior might find it confusing. Consider adding a tooltip or help text explaining this feature.
105-143: Search wrapping with wildcards may be too permissive.Line 118 wraps any non-wildcard search with
*search*, which could match a very large number of indices. Consider whether a prefix match (search*) would be more appropriate for most use cases, or make the matching strategy configurable.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.jssrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsxsrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_selector.scsssrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.scsssrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsxsrc/plugins/dataset_management/public/plugin.tssrc/plugins/explore/public/components/trace_auto_detect_callout.tsxsrc/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
💤 Files with no reviewable changes (1)
- src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_selector.scss
✅ Files skipped from review due to trivial changes (1)
- src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.scss
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/explore/public/components/trace_auto_detect_callout.tsx
- cypress/integration/core_opensearch_dashboards/opensearch_dashboards/apps/explore/01/queries.spec.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx (2)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/use_index_fetcher.ts (1)
useIndexFetcher(28-144)src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/constants.ts (1)
MAX_INITIAL_RESULTS(10-10)
src/plugins/dataset_management/public/plugin.ts (1)
src/core/public/index.ts (2)
AppUpdater(166-166)AppNavLinkStatus(164-164)
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (2)
src/plugins/explore/public/utils/auto_detect_trace_data.ts (1)
detectTraceData(24-111)src/plugins/explore/public/utils/create_auto_datasets.ts (1)
createAutoDetectedDatasets(18-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Lint and validate
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
🔇 Additional comments (12)
src/plugins/dataset_management/public/plugin.ts (4)
7-8: LGTM!Appropriate imports for the reactive workspace subscription pattern and dynamic app updates.
Also applies to: 20-26
64-65: LGTM!BehaviorSubject provides the correct semantics for app updates, and optional subscription enables proper lifecycle management.
143-170: LGTM!The
updater$binding enables dynamic nav link visibility based on workspace context.
206-212: LGTM!Proper subscription cleanup prevents memory leaks. Good practice to set the subscription to
undefinedafter unsubscribing.src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (1)
264-275: Deps look consistent with new services usage.Adding
savedObjectsanddataPluginto the callback deps matches the new flow.src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (3)
56-68: Well-structured state management with Sets for loading tracking.The use of
Setfor tracking loading states (loadingInfoForIndexSet,loadingMatchingIndicesSet) is an efficient pattern that prevents duplicate fetches and race conditions.
173-196: Auto-fetch logic properly prevents duplicate requests.The effect correctly checks both the cache and loading state before triggering fetches, avoiding unnecessary network calls and race conditions.
249-263: Data structure logic correctly handles mixed wildcard and exact selections.The conditional logic properly constructs
isMultiWildcardstructures with optional exact indices, orisMultiIndexstructures when no wildcards are present. This supports the flexible selection model.src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx (4)
60-103: Comprehensive validation logic for index patterns.The validation correctly identifies illegal characters and spaces, with clear, internationalized error messages. The separation of validation logic and message generation is clean.
190-197: Nice UX touch for cursor positioning.The automatic cursor repositioning after wildcard insertion ensures users can continue typing their pattern naturally. The use of a ref flag to coordinate this across render cycles is clean.
225-248: Duplicate checking prevents redundant selections.The function correctly checks for existing patterns before adding, preventing duplicate entries in the selection list.
153-178: Debounce cleanup is incomplete.The effect cleans up the timer when dependencies change, but there's no cleanup on component unmount. If the component unmounts while a debounce timer is pending, it could attempt to call
fetchIndicesafter unmount.🐛 Add unmount cleanup
The cleanup function in the effect will run on unmount, so this is actually fine. However, you should also clear the ref:
// Cleanup return () => { if (debounceTimerRef.current) { clearTimeout(debounceTimerRef.current); + debounceTimerRef.current = null; } };Likely an incorrect or invalid review comment.
...ery/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx
Show resolved
Hide resolved
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
Show resolved
Hide resolved
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
Show resolved
Hide resolved
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
Show resolved
Hide resolved
| </EuiText> | ||
| </EuiPanel> | ||
| ) : ( | ||
| <EuiPanel |
There was a problem hiding this comment.
I am curious how come we didn't use the OUI table component for this?
@ps48 @TackAdam |
...ery_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx
Outdated
Show resolved
Hide resolved
src/plugins/explore/public/components/trace_auto_detect_callout.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsx:
- Around line 202-235: The test declares an unused mock variable
`mockOnSelectionChange` which should be removed to avoid lint/test warnings;
delete the `const mockOnSelectionChange = jest.fn();` line in the 'Mixed
Selection (Single + Wildcard)' test and ensure no other references to
`mockOnSelectionChange` remain (keep the rest of the test intact, including
assertions that reference `mockSelectDataStructure` and
DATA_STRUCTURE_META_TYPES).
In @src/plugins/data/server/index_patterns/routes.ts:
- Around line 157-175: The handler currently allows an empty indices array which
makes indices.join(',') an empty string and causes
callAsCurrentUser('cat.indices', ...) to return info for all cluster indices;
fix by enforcing a non-empty indices array—update the request validation to
schema.arrayOf(schema.string(), { minSize: 1 }) for request.body.indices, and/or
add a runtime guard after const { indices } = request.body that checks if
(!indices || indices.length === 0) and returns response.badRequest({ body:
'indices must contain at least one entry' }); ensure the subsequent
callAsCurrentUser('cat.indices', { index: indices.join(','), ... }) only runs
when indices is non-empty.
🧹 Nitpick comments (5)
src/plugins/data/server/index_patterns/routes.ts (1)
183-190: Error handling looks reasonable but consider logging for observability.The fallback to status 500 and a generic message is appropriate. However, for production debugging, consider logging the original error before returning the sanitized response. This helps with troubleshooting without exposing internal details to clients.
Optional: Add server-side logging
} catch (error) { + // Log the full error server-side for debugging + context.core.opensearch.legacy.client.log?.error?.( + `cat.indices failed: ${error.message}`, + { indices: request.body.indices } + ); return response.customError({ statusCode: error.statusCode || 500, body: { message: error.message || 'Error fetching index information', }, }); }src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (3)
205-207: Silent error swallowing hinders debugging.Empty catch blocks make it difficult to diagnose issues when trace detection fails for a specific data source. Consider logging errors at minimum.
Proposed fix
} catch (error) { - // Continue with other data sources even if one fails + // Continue with other data sources even if one fails + // eslint-disable-next-line no-console + console.warn(`Trace detection failed for data source ${dataSourceId}:`, error); }
198-203: Dataset count may be misleading when datasets already exist.
datasetsCreatedCountincrements whenevercreateAutoDetectedDatasetsis called, but per thecreate_auto_datasets.tsimplementation, the function reuses existing datasets rather than creating new ones. This could show "Trace datasets created automatically" even when no new datasets were actually created.Consider checking the return value from
createAutoDetectedDatasetsto determine if new datasets were created:Proposed approach
const createResult = await createAutoDetectedDatasets( savedObjects.client, detection, dataSourceId ); - datasetsCreatedCount++; + // Only count if new datasets were actually created (not reused) + if (createResult.traceDatasetId || createResult.logDatasetId) { + datasetsCreatedCount++; + }Note: This still may count reused datasets. For accurate messaging,
createAutoDetectedDatasetswould need to return a flag indicating whether datasets were newly created vs. reused.
182-208: Sequential detection loop could benefit from parallelization.The detection and dataset creation for multiple data sources runs sequentially. For workspaces with several data sources, this adds latency before the redirect. Consider using
Promise.allSettledto parallelize the detection across data sources.Proposed parallel approach
- // Run detection for each data source - for (const dataSourceId of dataSourcesToCheck) { - try { - const detection = await detectTraceData( - savedObjects.client, - dataPlugin.dataViews, - dataSourceId - ); - - if (detection.tracesDetected || detection.logsDetected) { - // Add datasource title to detection - if (dataSourceId) { - detection.dataSourceTitle = dataSourceIdToName.get(dataSourceId); - } else { - detection.dataSourceTitle = 'Local Cluster'; - } - - await createAutoDetectedDatasets( - savedObjects.client, - detection, - dataSourceId - ); - datasetsCreatedCount++; - } - } catch (error) { - // Continue with other data sources even if one fails - } - } + // Run detection for all data sources in parallel + const detectionPromises = dataSourcesToCheck.map(async (dataSourceId) => { + const detection = await detectTraceData( + savedObjects.client, + dataPlugin.dataViews, + dataSourceId + ); + + if (detection.tracesDetected || detection.logsDetected) { + if (dataSourceId) { + detection.dataSourceTitle = dataSourceIdToName.get(dataSourceId); + } else { + detection.dataSourceTitle = 'Local Cluster'; + } + await createAutoDetectedDatasets(savedObjects.client, detection, dataSourceId); + return true; + } + return false; + }); + + const results = await Promise.allSettled(detectionPromises); + datasetsCreatedCount = results.filter( + (r) => r.status === 'fulfilled' && r.value + ).length;src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (1)
278-296: Consider guarding against state updates after unmount in async handler.
handleInfoIconClickis async and awaits fetch operations. If the component unmounts during the await, subsequent state updates (setting popover index) could cause issues. While React 18 mitigates most warnings, anisMountedref pattern would make this more robust.♻️ Optional: Add isMounted guard
+ const isMountedRef = useRef(true); + useEffect(() => { + return () => { isMountedRef.current = false; }; + }, []); const handleInfoIconClick = async (itemIndex: number, item: SelectedItem) => { if (openPopoverIndex === itemIndex) { setOpenPopoverIndex(null); } else { setOpenPopoverIndex(itemIndex); if (item.isWildcard) { if (!matchingIndicesCache[item.title]) { await fetchMatchingIndices(item.title); } } else { if (indexInfoCache[item.title] === undefined) { await fetchBatchIndexInfo([item.title]); } } } };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsxsrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsxsrc/plugins/data/server/index_patterns/routes.tssrc/plugins/explore/public/components/trace_auto_detect_callout.tsxsrc/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/explore/public/components/trace_auto_detect_callout.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (3)
src/plugins/data/common/datasets/types.ts (2)
DataStructureCreatorProps(37-52)DataStructure(132-157)src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/use_index_fetcher.ts (1)
useIndexFetcher(28-144)src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx (1)
UnifiedIndexSelector(38-415)
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (2)
src/plugins/explore/public/utils/auto_detect_trace_data.ts (1)
detectTraceData(24-111)src/plugins/explore/public/utils/create_auto_datasets.ts (1)
createAutoDetectedDatasets(18-231)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: bundle-analyzer
- GitHub Check: lighthouse
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Windows
🔇 Additional comments (14)
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx (3)
29-29: LGTM!Imports are correctly structured for the new trace auto-detection functionality, properly bringing in the data plugin type and the detection/creation utilities from the explore plugin.
Also applies to: 40-40
58-66: LGTM!The hook type annotation and destructuring are properly aligned. Using
dataPluginas an alias for thedataservice is a good practice to avoid naming conflicts.
267-278: LGTM!The dependency array correctly includes
savedObjectsanddataPluginsince they're now used within thehandleWorkspaceFormSubmitcallback for the trace auto-detection logic.src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (8)
6-30: LGTM!Imports are well-organized and include all necessary EUI components, i18n utilities, and internal types for the unified selector implementation.
36-69: Good pattern for tracking concurrent requests with Set-based state.The use of
Set<string>forloadingInfoForIndexSetandloadingMatchingIndicesSeteffectively prevents duplicate concurrent requests and allows efficient membership checking. The pagination state per wildcard pattern is a nice touch for the popover UX.
73-133: Batch fetching implementation is solid with proper loading state management.The approach of batching index info requests, caching results, and marking missing indices as
nullis efficient. Thefinallyblock ensures loading state is always cleared.One consideration: async operations without abort controllers could result in state updates after unmount. This is largely mitigated in React 18, but if you encounter console warnings in testing, adding an
AbortControllerto the HTTP request would address it.
136-165: LGTM!Clean implementation with proper validation, caching, and error handling. The early return for non-wildcard patterns prevents unnecessary requests.
167-217: Well-designed auto-fetch logic with cache-aware efficiency.The dual
useEffectpattern appropriately separates concerns: one for selected items and one for visible popover indices. The cache and loading-state checks prevent redundant requests. Dependency arrays are comprehensive.
219-264: Data structure construction logic is correct and handles all selection scenarios.The conditional metadata (
isMultiWildcardvsisMultiIndex) properly reflects the selection type, and mixed selections correctly include bothwildcardPatternsandselectedIndices/selectedTitles.
483-494: Consider adding a fallback for unexpected health values.The health color mapping defaults to
'danger'for any value that isn't'green'or'yellow'. This includes cases wherehealthmight be an unexpected value like'red'(correct), but also edge cases like an empty string or unexpected API responses.The current behavior is fail-safe, which is reasonable. Just noting this is intentional defensive coding.
509-727: LGTM!The render logic is clean and well-organized. Good use of EUI components, proper accessibility attributes (
aria-label), and clear empty state messaging. The table structure for selected items provides a consistent UX with the wildcard popover.src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsx (3)
19-59: Well-structured mock for UnifiedIndexSelector.The mock effectively simulates the component's behavior by exposing
selectedItemsandonSelectionChange, and provides action buttons for test interaction. This allows testing the parent component's logic without coupling to the actual selector implementation.
237-287: Thorough health data fetching tests.Good coverage of the API call parameters including
data_sourcequery param, request body, and error handling. The test at line 256-268 properly verifies graceful degradation when the fetch fails.
306-348: LGTM!Data source handling and services prop tests provide good coverage for edge cases: fallback to
'local'when path is empty, correct id extraction from DATA_SOURCE path item, and resilience whenservicesis undefined.
...tring/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsx
Show resolved
Hide resolved
src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx
Show resolved
Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (3)
114-122: Silent failure may leave users confused when health data cannot be fetched.When the batch fetch fails, errors are only logged to the console. Consider showing a toast notification to inform users that health data is unavailable, especially for network errors that might be transient.
💡 Optional: Add user notification on fetch error
} catch (error) { // eslint-disable-next-line no-console console.error('Error fetching batch index info:', error); + // Optionally notify user + services?.notifications?.toasts?.addWarning({ + title: i18n.translate('data.datasetService.indexInfo.fetchError', { + defaultMessage: 'Unable to fetch index health information', + }), + }); // Mark all as null on error const errorCache: Record<string, IndexInfo | null> = {};
483-494: Extract duplicated health color mapping to a helper.The health-to-color mapping logic is duplicated in two places: the health popover (lines 484-490) and the table row (lines 654-658). Consider extracting this to a helper function.
♻️ Extract helper function
+const getHealthColor = (health: string): 'success' | 'warning' | 'danger' => { + if (health === 'green') return 'success'; + if (health === 'yellow') return 'warning'; + return 'danger'; +}; // Then use it: <EuiHealth - color={ - infoData.health === 'green' - ? 'success' - : infoData.health === 'yellow' - ? 'warning' - : 'danger' - } + color={getHealthColor(infoData.health)} >Also applies to: 652-662
354-372: Inconsistent internationalization in popover content.The main table headers use
i18n.translate()correctly, but the popover content has hardcoded strings like "Name", "Documents", "Size", "Health:", "Status:" that should also be internationalized for consistency.🌐 Add i18n for popover strings
<EuiText size="xs"> - <strong>Name</strong> + <strong> + {i18n.translate('data.datasetService.unifiedSelector.popoverNameHeader', { + defaultMessage: 'Name', + })} + </strong> </EuiText>Apply similar changes to "Documents", "Size", "Health:", and "Status:" strings.
Also applies to: 479-504
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsx (2)
202-234: Remove unused variablemockOnSelectionChange.Line 204 declares
mockOnSelectionChangebut it's never used in this test.🧹 Remove unused variable
describe('Mixed Selection (Single + Wildcard)', () => { it('handles mixed selection of single indices and wildcards', async () => { - const mockOnSelectionChange = jest.fn(); const { getByTestId } = renderComponent();
96-347: Consider adding tests for item removal behavior.The test suite covers selection and DataStructure creation well, but doesn't test the removal functionality (
handleRemoveItem). Consider extending the mock to expose a remove action, or adding integration tests that render the actual selected items table.💡 Example: Extend mock to test removal
// In the mock, add a remove button for each item: {selectedItems.map((item: any, idx: number) => ( <div key={item.id} data-test-subj={`selected-item-${item.title}`}> {item.title} <button data-test-subj={`remove-item-${item.title}`} onClick={() => onSelectionChange(selectedItems.filter((_: any, i: number) => i !== idx))} > Remove </button> </div> ))}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsxsrc/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (5)
src/plugins/data/common/datasets/types.ts (2)
DataStructureCreatorProps(37-52)DataStructure(132-157)src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/use_index_fetcher.ts (1)
useIndexFetcher(28-144)src/plugins/vis_type_timeseries/public/lib/process_math_series.test.js (1)
response(10-10)cypress/utils/commands.explore.js (1)
pattern(64-64)src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/unified_index_selector.tsx (1)
UnifiedIndexSelector(38-415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (68)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: linkchecker
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: lighthouse
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Lint and validate
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
🔇 Additional comments (8)
src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.tsx (5)
56-68: Consider using functional updates for Set state to avoid stale closures.The
Setstates are being managed correctly with functional updates, but initializing withnew Set()on every render in the default state is fine. The implementation looks correct.LGTM! The state initialization properly handles caching, loading states, and pagination for the new unified selector pattern.
167-190: LGTM! The auto-fetch logic correctly checks both cache and loading state before triggering fetches, preventing duplicate requests. The fire-and-forget pattern is appropriate here since results are cached.
237-259: LGTM! The DataStructure construction logic correctly handles the three scenarios: wildcards only, exact indices only, and mixed selections. The conditional spread pattern for including exact indices with wildcards is clean.
266-276: LGTM! Good edge case handling - correctly adjustsopenPopoverIndexwhen removing an item that precedes the currently open popover.
597-722: LGTM! The table row rendering handles all cases correctly with appropriate loading states, empty state fallbacks, and accessibility attributes (aria-labels on action buttons).src/plugins/data/public/query/query_string/dataset_service/lib/index_data_structure_creator/index_data_structure_creator.test.tsx (3)
19-59: LGTM! The mock effectively isolates the component under test while providing hooks to verify the integration behavior. The mock's buttons allow simulating user interactions and the rendered selected items enable assertions.
146-155: Test adds duplicate items due to mock behavior.This test clicks
add-single-indextwice, which adds the sameindex1item twice. While this tests the parent component's ability to handle arrays, it may not reflect realistic behavior ifUnifiedIndexSelectorprevents duplicates. Consider either:
- Documenting this as intentional behavior testing, or
- Modifying the mock to add unique items (e.g.,
index1,index2)
237-287: LGTM! Good coverage of health data fetching scenarios including success path, error handling, and URL construction with data source ID. The error handling test (lines 256-268) correctly verifies the component remains functional after a fetch error.
6 participants
Description
Dataset Management Enhancements for Observability
This PR introduces significant improvements to dataset management in OpenSearch Dashboards, focusing on observability workflows, UI/UX enhancements, and intelligent automation for trace and log dataset creation.
1. Enable Dataset Management Only in Observability Workspace
Overview
Dataset management is now exclusively enabled in observability workspaces, providing a focused and streamlined experience for observability use cases while maintaining index pattern management for other workspace types.
Workspace.mov
Key Changes
Workspace-Aware Dataset Management
use-case-observabilityfeature) display dataset managementIndex Pattern Management Coordination
Technical Implementation
src/plugins/dataset_management/public/plugin.ts: Added workspace subscription with proper lifecycle managementsrc/plugins/workspace/public/plugin.ts: Enhanced workspace plugin dependencies and type safetysrc/plugins/index_pattern_management/public/plugin.ts: Coordinated app registration based on workspace capabilitiessrc/plugins/workspace/common/constants.ts: Added observability feature flag constants2. Updated Dataset Creation UI
Overview
Completely redesigned the dataset creation interface to provide a unified, intuitive experience for creating index patterns with single or multiple wildcard patterns.
DataSetUi.mov
Key Changes
Unified Index Selector
index_selector,mode_selection_row,matching_indices_list,multi_wildcard_selector) with a single, cohesiveunified_index_selectorEnhanced Main Creator Component
index_data_structure_creator.tsxto use the new unified selectorPerformance Optimizations
isMountedchecks to prevent updates after unmountUI/UX Improvements
$euiSizeXSinstead of$euiSizeM/$euiSizeS) for more compact displayCode Quality
Files Changed
unified_index_selector.tsx,unified_index_selector.test.tsx,unified_index_selector.scssindex_selector.tsx,matching_indices_list.tsx,mode_selection_row.tsx,multi_wildcard_selector.tsxindex_data_structure_creator.tsx,dataset_select.tsx,dataset_explorer.tsx3. Trace/Log Dataset Automation
Overview
Introduced intelligent auto-detection and automated creation of trace and log datasets when OpenTelemetry-compatible data is detected, both during workspace creation and when navigating to the Explore page.
AutoGenerateTrace.mov
Key Features
Auto-Detection System
traceId,spanId,durationInNanos,serviceNamefieldsotel-v1-apm-span*,logs-otel-v1*)Automated Dataset Creation
During Workspace Creation:
signalType: 'traces'signalType: 'logs'and correlation schema mappingsOn Explore Page Navigation:
Technical Implementation
Auto-Detection Logic (
auto_detect_trace_data.ts):Dataset Creation (
create_auto_datasets.ts):{ "otelLogs": { "timeField": "detected_time_field", "traceId": "traceId", "spanId": "spanId", "serviceName": "resource.attributes.service.name" } }UI Components:
trace_auto_detect_callout.tsx: Dismissible callout banner with loading statesbottom_right_container.tsx(Explore page)workspace_creator.tsx(Workspace creation flow)Error Handling & Edge Cases:
Test Coverage:
Benefits
Testing Improvements
Cypress Test Stability
Fixed multiple timing-related test failures in CI environments:
Toast Notification Timeouts:
saveQueryandupdateSavedQuerycommandsLoad Query Waits:
Page Reload Handling:
cy.reload()operationsNavigation Waits:
Files Updated:
cypress/utils/commands.explore.jscypress/utils/apps/data_explorer/commands.jscypress/integration/.../queries.spec.jscypress/integration/.../saved_explore.spec.jscypress/integration/.../saved_queries.spec.jsBreaking Changes
None. All changes are backward compatible. Workspaces without observability features continue to use index pattern management as before.
Migration Notes
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.