[Streams] Use TS instead of FROM for TSDB mode in Discover links#250554
[Streams] Use TS instead of FROM for TSDB mode in Discover links#250554flash1293 merged 11 commits intoelastic:mainfrom
Conversation
When a stream is in time-series (TSDB) mode, the "Explore in Discover" links should use `TS` instead of `FROM` in the ES|QL query. This change: - Creates a new `useStreamTSDBMode` hook that creates a DataView and checks if the stream has TSDB characteristics - Updates 4 components to use this hook and conditionally use TS/FROM: - streams_list/index.tsx (StreamNode) - stream_chart_panel.tsx - stream_badges/index.tsx (DiscoverBadgeButton) - system_events_data.tsx The failure store redirect link is intentionally not updated as the failure store is a separate index that doesn't share TSDB characteristics. Closes elastic#246220
|
/ralph Investigate how to send this information along with the streams listing instead of doing another request to fetch this information. Then update the PR with the new version. |
1 similar comment
|
/ralph Investigate how to send this information along with the streams listing instead of doing another request to fetch this information. Then update the PR with the new version. |
Instead of making N additional DataView requests to determine TSDB mode for each stream in listing views, use the index_mode field already available from the internal streams listing API response. - Add optional isTSDBMode prop to DiscoverBadgeButton - Pass TSDB mode from data_stream?.index_mode === 'time_series' in listing views - Update child_stream_list to fetch from internal API with data_stream info - Detail views continue using useStreamTSDBMode hook (single stream, acceptable)
…de hook This removes the expensive useStreamTSDBMode hook that created DataViews to check if a stream is in TSDB mode. Instead, we now: 1. Added index_mode field to the public stream detail API response 2. Components use definition.index_mode === 'time_series' directly 3. Deleted the useStreamTSDBMode hook This improves performance by eliminating unnecessary DataView creation and ES calls when viewing stream details.
| export function DiscoverBadgeButton({ | ||
| definition, | ||
| isWiredStream, | ||
| isTSDBMode: isTSDBModeProp, |
There was a problem hiding this comment.
@copilot This prop is not used anywhere, please remove it
|
/ralph It looks like the DiscoverBadgeButton doesn't need the isTSDBMode prop anymore - verify this is the case and revert this change if true. Then update the PR |
The prop was unnecessary because: - definition.index_mode is already available from the API response - In tree_table.tsx, we can include index_mode directly in the constructed definition object instead of passing a separate prop
|
Removed the redundant Why it was unnecessary:
Changes:
Validation: Written by cursor, wait for @flash1293 to chime in |
|
Pinging @elastic/obs-onboarding-team (Team:obs-onboarding) |
|
/ralph there is a type error: |
Changed import to use value import for Streams (needed for runtime .is() method) while keeping System as a type-only import using inline type modifier. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed the TypeScript import error in Problem: Solution: Changed the import from: import type { Streams, System } from '@kbn/streams-schema';to: import { Streams, type System } from '@kbn/streams-schema';This keeps Written by cursor, wait for @flash1293 to chime in |
|
/ralph there are linting errors: Fix them and push the fix to the PR |
- Remove unused catch clause variables (use empty catch) - Move beforeEach hook before afterAll hook - Remove unused page parameter from test function Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed the 5 linting errors in
Written by cursor, wait for @flash1293 to chime in |
There was a problem hiding this comment.
Pull request overview
This PR updates the Streams app to use TS (time series) instead of FROM in ES|QL queries when linking to Discover for streams in TSDB mode, enabling better query optimization for time-series data.
Changes:
- Added
index_modefield to stream API responses to expose TSDB mode information - Updated 4 components generating Discover links to conditionally use
TSorFROMbased on TSDB mode - Added Scout/Playwright integration tests validating correct source command usage for TSDB vs regular streams
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
tsdb_discover_links.spec.ts |
New integration test validating TSDB-aware Discover links for both TSDB and regular streams |
streams_app.ts |
Added helper method to extract source command from Discover button links and updated existing verification method |
streams_list/index.tsx |
Updated StreamNode to use index_mode from data stream for TSDB detection and conditional source command |
tree_table.tsx |
Added index_mode field to stream item passed to child components |
system_events_data.tsx |
Updated to use index_mode from stream detail context for TSDB-aware query generation |
stream_chart_panel.tsx |
Updated to use index_mode from definition for TSDB-aware base queries |
child_stream_list.tsx |
Refactored to fetch streams with data_stream info for TSDB mode detection in child streams |
stream_badges/index.tsx |
Updated DiscoverBadgeButton to use index_mode for TSDB-aware query generation |
read_stream.ts |
Added index_mode field extraction from data stream to API responses |
base.ts |
Added IngestStreamIndexMode type and index_mode field to schema definitions |
index.ts |
Exported IngestStreamIndexMode type for external use |
bundle.serverless.json |
Updated maxItems from 1000 to 10000 (unrelated change) |
bundle.json |
Updated maxItems from 1000 to 10000 (unrelated change) |
Comments suppressed due to low confidence (2)
oas_docs/bundle.serverless.json:11467
- The maxItems change from 1000 to 10000 appears unrelated to TSDB Discover links. This change should either be removed from this PR or explained in the PR description as it affects API schema validation limits.
"maxItems": 10000,
oas_docs/bundle.json:11467
- The maxItems change from 1000 to 10000 appears unrelated to TSDB Discover links. This change should either be removed from this PR or explained in the PR description as it affects API schema validation limits.
"maxItems": 10000,
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
History
|
| return { | ||
| stream: streamDefinition, | ||
| privileges, | ||
| index_mode: dataStream?.index_mode as IngestStreamIndexMode | undefined, |
There was a problem hiding this comment.
nit: why is this type assertion necessary? It shouldn't be harmful, but it's always nice to prevent assertions as you won't be protected if index_mode changes or is removed.
There was a problem hiding this comment.
Good call, we should change that.
| ); | ||
|
|
||
| // Use TSDB mode from data_stream.index_mode directly from listing data | ||
| const isTSDBMode = node.data_stream?.index_mode === 'time_series'; |
There was a problem hiding this comment.
I've seen this about 4 times in the PR and it will probably used more, can we expose a isTSDBMode util from the streams package?
There was a problem hiding this comment.
Agreed, we should centralize. We should even go a bit further and centralize the whole base ESQL query generation (definition -> query) instead of having that logic in multiple places, there is no good reason for that.
|
/ralph Check the review comments of this PR and what flash1293 replied to the points that were raised. Implement these changes, check linting and type checks and push the fix on the PR |
…ilities - Remove unnecessary type assertion in read_stream.ts (IndicesIndexMode and IngestStreamIndexMode are structurally identical) - Delete unused useWiredStreams.ts hook (dead code) - Add isTSDBMode utility function in @kbn/streams-schema for checking time-series mode - Add getDiscoverEsqlQuery utility function to centralize ESQL query generation from stream definitions, handling FROM/TS source commands based on index mode - Update streams_list, stream_badges, system_events_data, and stream_chart_panel to use the new utilities Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the review comments from @tonyghiani: Changes:
All linting and type checks pass. Written by cursor, wait for @flash1293 to chime in |
|
/ralph now that isTSDBMode is only used from one place, you can inline it into getDiscoverEsqlQuery. Also check how you could consolidate it with the existing buildEsqlQuery if possible. If there isn't an approach that makes sense and makes the code cleaner and leaner, that's also fine, we can leave it for another time. Please update the PR |
|
@tonyghiani what do you think about having the ESQL query builder helper in streams-schema? I don't love the location, but I also don't know where else to put it tbh. We already have a couple of related things in there, so 🤷 |
I think we should use it, manually writing our queries doesn't scale and doesn't inherit changes to the language syntax that the query builder would support out of the box. WRT the location, I agree the streams-schema it's not the perfect fit, but looks ok if put close to the other related stuff. |
- Removed the separate isTSDBMode helper function since it was only used in one place - Inlined the logic directly in getDiscoverEsqlQuery - Removed the export from kbn-streams-schema index - Deleted the is_tsdb_mode.ts helper file Co-authored-by: Cursor <cursoragent@cursor.com>
|
Inlined Changes:
Regarding consolidation with Written by cursor, wait for @flash1293 to chime in |
|
@tonyghiani should be good for another look |
tonyghiani
left a comment
There was a problem hiding this comment.
Looks good, thanks for the change!
…stic#250554) ## 🍒 Summary When a stream is in time-series (TSDB) mode, the "Explore in Discover" links should use `TS` instead of `FROM` in the ES|QL query to properly leverage TSDB optimizations. Closes elastic#246220 ## 🛠️ Changes - Created a new `useStreamTSDBMode` hook that creates a DataView for a stream and checks `dataView.isTSDBMode()` to determine if the stream has TSDB characteristics (time series dimensions/metrics) - Updated 4 components that generate "Open in Discover" links to use `TS` or `FROM` based on the stream's TSDB mode: - `streams_list/index.tsx` (StreamNode component) - `stream_detail_overview/components/stream_chart_panel.tsx` - `stream_badges/index.tsx` (DiscoverBadgeButton) - `stream_detail_systems/stream_systems/system_events_data.tsx` - Added Scout/Playwright UI integration tests to validate TSDB-aware Discover links - Note: The failure store redirect link (`use_failure_store_redirect_link.ts`) was intentionally not updated as the failure store is a separate index with a `-failures` suffix that doesn't share TSDB characteristics with the main stream ## 🎙️ Prompts - "When a stream is in time-series (TSDB) mode, clicking 'Explore in Discover' should use TS instead of FROM in the ES|QL query" 🤖 This pull request was assisted by Cursor --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds a new IngestStreamIndexMode type and an optional index_mode field on ingest stream responses. Introduces getDiscoverEsqlQuery to produce ES|QL queries (using TS for time-series streams, FROM otherwise) and re-exports it from the shared schema package. Server read_stream responses now propagate dataStream.index_mode. Multiple frontend components were updated to use getDiscoverEsqlQuery instead of manual index-pattern assembly. A TSDB-focused test suite and fixture updates verify Discover links use the correct source command. One removed hook (useWiredStreams) and various UI type/shape updates accompany these changes. Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
✅ Actions performedReview triggered.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR introduces index_mode tracking and updates ES|QL query generation across the streams system. It adds a new IngestStreamIndexMode type supporting 'standard', 'time_series', 'logsdb', and 'lookup' modes, creates a getDiscoverEsqlQuery helper that generates appropriate ES|QL queries (using 'TS' for time_series mode, 'FROM' otherwise), and propagates index_mode through the streams API responses. Multiple components are refactored to use the new helper instead of manual query construction, and data fetching is restructured to obtain index_mode information. Test infrastructure is updated to verify correct source command selection based on stream mode. Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
🍒 Summary
When a stream is in time-series (TSDB) mode, the "Explore in Discover" links should use
TSinstead ofFROMin the ES|QL query to properly leverage TSDB optimizations.Closes #246220
🛠️ Changes
useStreamTSDBModehook that creates a DataView for a stream and checksdataView.isTSDBMode()to determine if the stream has TSDB characteristics (time series dimensions/metrics)TSorFROMbased on the stream's TSDB mode:streams_list/index.tsx(StreamNode component)stream_detail_overview/components/stream_chart_panel.tsxstream_badges/index.tsx(DiscoverBadgeButton)stream_detail_systems/stream_systems/system_events_data.tsxuse_failure_store_redirect_link.ts) was intentionally not updated as the failure store is a separate index with a-failuressuffix that doesn't share TSDB characteristics with the main stream🎙️ Prompts
🤖 This pull request was assisted by Cursor