[Explore] add dataset signal type to avoid prometheus in logs #11033
Conversation
WalkthroughThis PR adds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (2)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (2)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (2)
⏰ 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)
🔇 Additional comments (8)
Comment |
github-actions
bot
added
distinguished-contributor
Skip-Changelog
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11033 +/- ##
=======================================
Coverage 60.80% 60.80%
=======================================
Files 4538 4538
Lines 122703 122711 +8
Branches 20617 20625 +8
=======================================
+ Hits 74605 74611 +6
Misses 42813 42813
- Partials 5285 5287 +2
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:
|
joshuali925
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
LDrago27,
Maosaic,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
angle943,
ashwin-pc,
bandinib-amzn,
curq,
huyaboo,
joshuarrrr,
kavilla,
manasvinibs,
mengweieric,
ruanyl,
sejli,
virajsanghvi,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
joshuali925
requested review from
TackAdam,
d-buckner,
paulstn,
ps48,
raintygao,
ruchidh,
sumukhswamy and
wanglam
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/plugins/data/common/datasets/types.ts (1)
277-278: Consider using a stricter type forsignalType.The
signalTypeproperty uses a loosestringtype. Given thatCORE_SIGNAL_TYPESis defined insrc/plugins/data/common/index_patterns/types.tswith specific values ('logs','metrics','traces'), consider using a union type or the const type for better type safety:- /** Optional signal type for the dataset (e.g., 'logs', 'metrics', 'traces') */ - signalType?: string; + /** Optional signal type for the dataset (e.g., 'logs', 'metrics', 'traces') */ + signalType?: typeof CORE_SIGNAL_TYPES[keyof typeof CORE_SIGNAL_TYPES];This would require importing
CORE_SIGNAL_TYPESfrom../index_patterns/types. If extensibility for future signal types is intended, keepingstringis acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/plugins/data/common/datasets/types.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (3)
src/plugins/home/public/application/sample_data_client.test.js (1)
mockServices(38-44)src/plugins/data/common/index_patterns/types.ts (1)
CORE_SIGNAL_TYPES(41-45)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (1)
getPreloadedState(134-152)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (2)
src/plugins/data/public/index.ts (1)
QueryState(539-539)src/plugins/data/common/index_patterns/types.ts (1)
CORE_SIGNAL_TYPES(41-45)
🔇 Additional comments (6)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (1)
771-856: Good test coverage for Metrics flavor handling.The new test suite provides comprehensive coverage for Metrics flavor SignalType validation:
- Verifies Metrics datasets are accepted for Metrics flavor
- Verifies non-Metrics datasets are rejected for Metrics flavor
- Verifies Metrics datasets are rejected for Logs flavor
This mirrors the existing Traces flavor tests and ensures symmetric behavior.
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (5)
90-101: LGTM: Dataset change detection and language handling.The logic correctly detects when the dataset changes (by id or type) due to SignalType filtering and appropriately updates the language to match the resolved dataset. This ensures query compatibility when a dataset is swapped for flavor compliance.
236-241: Correct flavor-to-signalType mapping.The nested ternary correctly maps
ExploreFlavor.TracestoCORE_SIGNAL_TYPES.TRACESandExploreFlavor.MetricstoCORE_SIGNAL_TYPES.METRICS, withundefinedfor the Logs flavor (which uses exclusion logic instead).
204-211: Correct exclusion of TRACES and METRICS from Logs flavor.The updated logic ensures both TRACES and METRICS signalType datasets are excluded when no
requiredSignalTypeis specified (i.e., Logs flavor). This aligns with the PR objective to have Prometheus appear only under Metrics.
82-82: LGTM: signalType propagation in URL dataset.Including
signalTypein the URL dataset construction ensures the signal type is preserved when loading state from URL parameters.
301-308: LGTM: signalType included in minimal dataset construction.The
signalTypeis correctly propagated to the minimal dataset, ensuring it's preserved through the dataset resolution and preloaded state pipeline.
| // Get effective signal type from dataView or preferredDataset (for Prometheus which sets signalType directly) | ||
| const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType; | ||
|
|
There was a problem hiding this comment.
Potential bug: effectiveSignalType may not consider existingDataset.signalType.
At line 257, effectiveSignalType falls back to preferredDataset?.signalType, but existingDataset (line 246) could come from queryStringQuery?.dataset or defaultQuery?.dataset, not just preferredDataset. If the existing dataset from those sources has a signalType but preferredDataset is undefined, the fallback won't capture it.
Consider using existingDataset.signalType as the fallback:
// Get effective signal type from dataView or preferredDataset (for Prometheus which sets signalType directly)
- const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType;
+ const effectiveSignalType = dataView?.signalType || existingDataset.signalType;This ensures signalType is considered regardless of how existingDataset was resolved.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get effective signal type from dataView or preferredDataset (for Prometheus which sets signalType directly) | |
| const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType; | |
| // Get effective signal type from dataView or preferredDataset (for Prometheus which sets signalType directly) | |
| const effectiveSignalType = dataView?.signalType || existingDataset.signalType; |
🤖 Prompt for AI Agents
In
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts
around lines 256 to 258 (existingDataset is resolved around line 246),
effectiveSignalType currently uses dataView?.signalType ||
preferredDataset?.signalType and misses existingDataset.signalType; update the
fallback chain to include existingDataset.signalType (e.g., dataView?.signalType
|| preferredDataset?.signalType || existingDataset?.signalType) so the resolved
signalType covers datasets from queryStringQuery/defaultQuery as well as
preferredDataset.
7ed1b9b to
ab2c71b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (1)
256-258: TheeffectiveSignalTypefallback may missexistingDataset.signalType.As flagged in a previous review, when
preferredDatasetis undefined butexistingDatasetis resolved fromqueryStringQuery?.datasetordefaultQuery?.dataset(line 246), theirsignalTypewon't be considered in the fallback at line 257.// Get effective signal type from dataView or preferredDataset (for Prometheus which sets signalType directly) - const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType; + const effectiveSignalType = dataView?.signalType || existingDataset?.signalType;
🧹 Nitpick comments (1)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (1)
771-856: Good test coverage for Metrics flavor signalType handling.The tests cover the essential scenarios for
getPreloadedState. For completeness and consistency with the existingloadReduxState with SignalType validation for URL datasetstest suite (lines 586-768), consider adding a parallel test for Metrics flavor URL dataset validation (e.g., rejecting a Logs dataset URL for Metrics flavor and fetching a compatible Metrics dataset).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/plugins/data/common/datasets/types.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts(2 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/data/common/datasets/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (2)
src/plugins/data/public/index.ts (1)
QueryState(539-539)src/plugins/data/common/index_patterns/types.ts (1)
CORE_SIGNAL_TYPES(41-45)
⏰ 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:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: lighthouse
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: bundle-analyzer
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Lint and validate
🔇 Additional comments (7)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (1)
672-673: LGTM!The updated expectation correctly reflects the implementation behavior: when the dataset changes due to signal type filtering, the query is cleared since the previous query may be incompatible with the new dataset.
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (6)
82-82: LGTM!Correctly propagates
signalTypefrom the URL state to maintain consistency when reconstructing the dataset.
90-101: LGTM!The
datasetChangeddetection and query clearing logic ensures that incompatible queries aren't preserved when the dataset is swapped due to signal type filtering. This prevents runtime errors from executing queries against mismatched datasets.
195-211: LGTM!The
effectiveSignalTypelogic correctly handles both dataView-based signalType (for index patterns) and dataset-level signalType (for Prometheus which lacks a dataView). The filtering logic properly enforces signal type constraints for all flavors.
237-241: LGTM!The ternary chain correctly maps each flavor to its required signal type.
264-272: LGTM!The validation logic for Logs flavor correctly rejects both Traces and Metrics datasets, maintaining consistency with
fetchFirstAvailableDataset.
301-308: LGTM!Including
signalTypein theminimalDatasetensures that the signal type is preserved through the Redux state and URL persistence cycle.
joshuali925
force-pushed
the
dataset-signal-type
branch
from
ab2c71b to
65ea019
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (1)
248-272:effectiveSignalTypeshould also considerexistingDataset.signalType(reiterating earlier feedback)Here
effectiveSignalTypeis derived as:const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType;But
existingDatasetmay come fromqueryStringQuery?.datasetordefaultQuery?.dataset, and can already carry asignalType(especially for non‑index‑pattern datasets) even whenpreferredDatasetisundefined. In that case,effectiveSignalTypebecomesundefinedand the dataset is treated as “logs‑like”, potentially allowing a Metrics/Traces dataset to be used in the wrong flavor.To cover all sources of the resolved dataset, consider including
existingDataset.signalTypein the fallback chain, e.g.:- const effectiveSignalType = dataView?.signalType || preferredDataset?.signalType; + const effectiveSignalType = + dataView?.signalType || existingDataset.signalType || preferredDataset?.signalType;This way, datasets obtained from queryString/defaultQuery with a
signalTypeare validated consistently with those coming in aspreferredDataset.
🧹 Nitpick comments (3)
src/plugins/data/common/datasets/types.ts (1)
272-292: DatasetsignalTypeis fine; consider tying the type toCORE_SIGNAL_TYPESfor safetyAdding
signalType?: stringhere is a good, minimally invasive way to plumb signal type into datasets. To prevent accidental typos and keep this aligned withCORE_SIGNAL_TYPES, you might later tighten the type (e.g., aSignalTypeunion derived fromCORE_SIGNAL_TYPESinstead of barestring). Not urgent, just a future type-safety win.src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (1)
771-856: Metrics flavor tests look good; consider an additional case for dataset-onlysignalTypeThese three tests nicely cover the Metrics flavor matrix (accept Metrics, reject non‑Metrics, and reject Metrics when in Logs flavor) and line up with the signalType filtering logic in
fetchFirstAvailableDataset/resolveDataset.If you want to harden behavior around Prometheus‑style datasets that only set
signalTypeon the dataset (and may not have a real data view), an extra test wheredataViews.getreturns an object withoutsignalType(or is omitted), buttoDataset()returnssignalType: METRICS, would verify that path too.src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (1)
186-219: Consider falling back to datasetsignalTypeeven whendataViews.getfailsThe
effectiveSignalTypecalculation here correctly prefersdataView?.signalTypeand falls back todataset.signalTypewhendataViews.getsucceeds:const effectiveSignalType = dataView?.signalType || dataset.signalType;However, if
dataViews.getthrows (e.g., Prometheus datasets that truly have no data view), thecatchwillcontinueand this dataset will never be considered, even ifdataset.signalTypeis set. That slightly undermines the PR’s goal of supporting datasets “that lack an index-pattern or data view”.You might want to treat
dataset.signalTypeas the fallback even whendataViews.getfails, for example by computingeffectiveSignalTypeoutside thetryand only overriding it when a data view is successfully loaded.Would you confirm how
dataViews.getbehaves for Prometheus datasets in practice and whether it can throw for “no data view” cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/_sidebar.md(1 hunks)src/plugins/data/common/datasets/types.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts(2 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/_sidebar.md
🧰 Additional context used
🧬 Code graph analysis (2)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (2)
src/plugins/data/common/index_patterns/types.ts (1)
CORE_SIGNAL_TYPES(41-45)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (1)
getPreloadedState(134-152)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (2)
src/plugins/data/public/index.ts (1)
QueryState(539-539)src/plugins/data/common/index_patterns/types.ts (1)
CORE_SIGNAL_TYPES(41-45)
⏰ 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 functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Lint and validate
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: lighthouse
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
🔇 Additional comments (5)
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (1)
662-674: Test expectation for clearing query on dataset change matches the new behaviorThe assertion that
result.query.querybecomes''when an incompatible URL dataset is replaced is consistent withloadReduxStatenow resettingquerywhendatasetChangedis true. This looks correct and avoids replaying potentially incompatible queries across signal types.src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (4)
72-84: IncludingsignalTypeinurlDatasetkeeps URL and resolution paths consistentCopying
signalTypefromqueryState.datasetintourlDatasetensures that datasets which only encode their signal type on the Dataset (e.g., Prometheus) participate correctly in subsequent validation. This is consistent with howminimalDatasetis built ingetPreloadedQueryState.
89-101:datasetChanged+ query/language reset behavior is coherent with signalType filteringThe
datasetChangedcheck on{id, type}and the follow‑up logic:
- updating
languagefromresolvedQueryState.languagewhen the dataset changes, and- clearing
queryto''in that caseare aligned with the intent to treat a signalType‑driven dataset swap as a fresh context. This avoids running an old query against a semantically different dataset while still preserving URL‑supplied query/language when the dataset is unchanged.
236-241: Flavor →requiredSignalTypemapping is straightforward and matchesExploreFlavorThe mapping from
ExploreFlavor.Traces→CORE_SIGNAL_TYPES.TRACES,ExploreFlavor.Metrics→CORE_SIGNAL_TYPES.METRICS, andundefinedotherwise is clear and keeps Logs (and any other non‑Traces/Metrics flavors) in the “no required signal type” bucket. Looks good.
301-308: PropagatingsignalTypeintominimalDatasetkeeps downstream consumers consistentAdding
signalType: selectedDataset.signalTypeto theminimalDatasetensures that anything consuming the preloaded query state (includinggetInitialQueryByDatasetand URL serialization) has access to the dataset’s signal type, which matches the new URL handling and filtering logic. This keeps the different initialization paths aligned.
Signed-off-by: Joshua Li <[email protected]>
joshuali925
force-pushed
the
dataset-signal-type
branch
from
65ea019 to
9d504cd
Compare
1 participant
Description
add signal type to dataset interface. For prometheus, there's no index-pattern or dataview, but we still need some place to store the signal type, so that prometheus only shows up in metrics
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.