Conversation
… resolution Signed-off-by: Adam Tackett <tackadam@amazon.com>
PR Reviewer Guide 🔍(Review updated until commit cb5faea)Here are some key observations to aid the review process:
|
| isRemoteDataset: dataset.isRemoteDataset, | ||
| displayName: dataset.displayName, | ||
| description: dataset.description, | ||
| schemaMappings: dataset.schemaMappings, |
There was a problem hiding this comment.
p2: extractSerializableDataset omits sourceDatasetRef
The docstring says this copies "every serializable hydration field," but the Dataset type also has sourceDatasetRef (data/common/datasets/types.ts:301 - the reference an indexed view stores to its table dataset), and it isn't copied here. So a dataset of that kind loses sourceDatasetRef through persistence, which is the same class of lossy-serialization bug this PR is fixing. It wasn't preserved by the old hand-rolled reducers either, so no regression, but since this is now meant to be the single lossless reducer, could we add sourceDatasetRef: dataset.sourceDatasetRef (or drop the "every field" wording and note the intentional exclusions)?
There was a problem hiding this comment.
Added sourceDatasetRef to extractSerializableDataset (with a test asserting it survives persistence). The helper also now drops undefined keys (see the round-trip thread), so it is genuinely lossless for the fields a dataset actually carries. (commit 9c7e2b7)
| // `entities` holds the traces<->logs linkage read in checkCorrelationsForLogs and must be | ||
| // requested explicitly. `references` is a top-level saved-object property and is always | ||
| // returned regardless of `fields`. | ||
| fields: ['entities'], |
There was a problem hiding this comment.
p2: the sibling hasEntityReference check reads a path this fetch no longer populates
Good fix. One loose end from it: findCorrelationsByDataset (:100) still filters on correlationAttrs?.correlations?.entities, but the linkage actually lives under top-level entities (what checkCorrelationsForLogs reads at :201 and what this change now requests). With correlations no longer in fields, hasEntityReference is always undefined / false, so discovery relies entirely on hasReference (top-level references) - which works, so nothing breaks today. But the entity fallback is now dead. Since entities is fetched now, could we either align :100 to read correlationAttrs?.entities?.some(...) so the fallback actually functions, or remove it so the dead nested-path check doesn't mislead?
There was a problem hiding this comment.
Removed the hasEntityReference branch and the correlations?: member of CorrelationAttributes, so top-level references is now clearly the only linkage. (commit 9c7e2b7)
ps48
left a comment
There was a problem hiding this comment.
fix: hydrate dataset fields on load and fix trace-to-log correlation resolution
Two clean, well-isolated bug fixes, both verified. Fix 1: DataView.toDataset() now carries signalType, indexPatternTypeConfig.toDataset() carries schemaMappings, and fetchDefaultDataset threads both through DataStructure meta, so the fresh-load default dataset is fully hydrated without extra saved-object lookups. The new extractSerializableDataset helper is a good call: it replaces two hand-rolled minimal-dataset reducers plus the in-context-editor copy with one source, and it preserves strictly more than the old versions (adds isRemoteDataset, displayName, description, schemaMappings) while copying explicitly so class methods can't leak into serialized state. Fix 2 is correct: the resolution path in checkCorrelationsForLogs reads top-level attributes.entities, which was never in the requested fields, so it was always undefined; requesting ['entities'] fixes it, and top-level references (used for discovery and log-dataset resolution) is returned regardless of fields. Tests added for both. No new deps, routes, saved-object types, or stray console.* (the existing ones are in catch blocks).
No p0, no p1. Two p2s inline, both about completeness.
Maosaic
left a comment
There was a problem hiding this comment.
Both root causes look correctly diagnosed and the direction is right. I ran the affected suites locally on this branch (Node 22.23.0):
- The 3 suites touched by this PR: 71 passed
- Full
src/plugins/data+src/plugins/exploresweep: 1118 suites / 11415 tests / 501 snapshots, all passing — so addingsignalTypetoDataView.toDataset()doesn't break any of its other consumers (query_actions.ts:587,trace_query_actions.ts:115,dataset_select.tsx:762,antlr/shared/utils.ts:241,agent_traces).
Comments inline. The two I'd most want addressed before merge are the sourceDatasetRef omission and the undefined-key behavior in extractSerializableDataset — I verified both by running the real function, details in those threads.
Two more that are outside the diff but adjacent enough to mention:
convertToDataset's non-DataView fallback still drops fields — src/plugins/data/common/data_views/data_views/data_views.ts:803. It returns id/title/type/timeFieldName/displayName/description/dataSource but not signalType or schemaMappings. It's rarely hit (dataViews.get() returns a DataView, which takes the toDataset() branch), but it's the one converter this PR's "make each converter lossless" sweep didn't cover.
Missing await — trace_query_actions.ts:115:
const dataset = services.data.dataViews.convertToDataset(dataView); // returns a Promiseso preparedQueryObject.dataset is a Promise rather than a Dataset. Currently harmless because only .query is read downstream (line 139), but it's on the traces execution path this PR is fixing, and it'd bite the moment someone reads that field.
| // `entities` holds the traces<->logs linkage read in checkCorrelationsForLogs and must be | ||
| // requested explicitly. `references` is a top-level saved-object property and is always | ||
| // returned regardless of `fields`. | ||
| fields: ['entities'], |
There was a problem hiding this comment.
Good catch on the root cause. One loose end: findCorrelationsByDataset still branches on the attribute you just stopped requesting —
const hasEntityReference = correlationAttrs?.correlations?.entities?.some(
(entity: { id: string }) => entity.id === datasetId
);with fields: ['entities'], attributes.correlations is never returned, so hasEntityReference is now permanently false.
As far as I can tell there's no runtime regression, because that branch was already dead: the saved-object mapping in src/plugins/data/server/saved_objects/correlations.ts only defines title, correlationType, version and entities — there is no correlations attribute — and neither create_auto_datasets.ts nor dataset_management's correlations_client.ts ever writes one.
But leaving it in place is exactly how this bug comes back. Could you delete hasEntityReference and the correlations?: member of CorrelationAttributes in this PR? That makes it obvious that top-level references is the only linkage, which is the thing your comment above is explaining.
There was a problem hiding this comment.
Agreed — deleted hasEntityReference and the correlations?: member of CorrelationAttributes. The saved-object mapping only defines entities, so references is the single source of truth for the linkage now. (commit 9c7e2b7)
| // requested explicitly. `references` is a top-level saved-object property and is always | ||
| // returned regardless of `fields`. | ||
| fields: ['entities'], | ||
| perPage: size, |
There was a problem hiding this comment.
Pre-existing, but it's the same "No log datasets found for this span" symptom you're fixing, so worth folding in while you're here: this fetches at most size (default 10) correlation objects with no filter and then filters client-side. With more than 10 correlations in the workspace, the one that references this dataset simply may not be in the page.
savedObjectsClient.find supports hasReference (src/core/server/saved_objects/types.ts:100, mapped to has_reference in the public client), so this could be:
const allCorrelationsResponse = await this.savedObjectsClient.find({
type: 'correlations',
fields: ['entities'],
hasReference: { type: 'index-pattern', id: datasetId },
perPage: size,
});which makes the query both correct and cheaper, and lets the hasReference filter below go away entirely.
There was a problem hiding this comment.
Good call — switched to a server-side hasReference: { type: 'index-pattern', id: datasetId } filter and removed the client-side pass, so a correlation past the first page can no longer be missed. (commit 9c7e2b7)
| * persisting into query/URL state. | ||
| * | ||
| * This is the single place Explore reduces a dataset for persistence. All hydration fields | ||
| * (dataSource, displayName, signalType, schemaMappings, ...) must be carried through so consumers |
There was a problem hiding this comment.
All hydration fields (dataSource, displayName, signalType, schemaMappings, ...) must be carried through
The helper is one field short of that claim: sourceDatasetRef isn't copied. Checked against the Dataset interface (data/common/datasets/types.ts:296-310), it's the only member not covered.
It matters because configurator.tsx:164 sets it for indexed views, and both dataset_select.tsx:433 and dataset_selector.tsx:85 route on it:
datasetService.getType(selectedDataset?.sourceDatasetRef?.type || selectedDataset?.type || '')so after a reload that routing silently falls back to dataset.type.
I confirmed the drop by running the real function against an indexed-view-shaped dataset — sourceDatasetRef comes back undefined. Since this PR is establishing the helper as the single lossless reducer, it's worth closing with one more line.
There was a problem hiding this comment.
Added sourceDatasetRef — thanks for pinning it to the exact interface member. Also reworked the helper to strip undefined keys so the "carries every field" claim holds without emitting phantom keys. (commit 9c7e2b7)
| * receive a complete dataset from the persisted state. Fields are copied explicitly (rather than | ||
| * spreading) so a class instance's methods never leak into serialized state. | ||
| */ | ||
| export const extractSerializableDataset = (dataset: Dataset): Dataset => ({ |
There was a problem hiding this comment.
Copying fields unconditionally means the result always carries all 11 keys, including the undefined ones — which the new test locks in (isRemoteDataset: undefined, description: undefined). That interacts badly with the dataset deep-equality gates.
rison drops undefined keys on the way into the URL, so a dataset read back from _q has fewer keys than a freshly-extracted one, and lodash isEqual({a: undefined}, {}) is false. I measured it with the real function:
key count: extracted = 11 | after URL round-trip = 6
isEqual(extracted, roundTripped) = false
Five of eleven keys vanish. That's the exact comparison at middleware/dataset_change_middleware.ts:52, where a false negative dispatches clearResults, resetLegacyState, clearQueryStatusMap and setActiveTab. Same pattern at query_string_manager.ts:212 and query_builder.ts:294.
It is self-correcting — re-extracting the round-tripped object re-materializes the keys and then compares equal — so it's a first-comparison-after-load hazard rather than a loop, which is why the suite stays green. The previous inline reducer had the same shape for 7 keys, so this widens an existing hazard rather than creating one.
Stripping undefined before returning would remove the whole class of problem (and incidentally covers the sourceDatasetRef case above):
export const extractSerializableDataset = (dataset: Dataset): Dataset =>
Object.fromEntries(
Object.entries({ id: dataset.id, /* ...including sourceDatasetRef... */ })
.filter(([, v]) => v !== undefined)
) as Dataset;There was a problem hiding this comment.
Fixed — the helper now builds via Object.fromEntries(Object.entries({...}).filter(([, v]) => v !== undefined)), so a freshly-extracted dataset has the same key set as one read back from _q; the isEqual gates (dataset-change middleware et al.) no longer fire a spurious change on first comparison after load. Added a test locking in the dropped-undefined behavior. (commit 9c7e2b7)
| ...(patternMeta?.signalType && { signalType: patternMeta.signalType }), | ||
| ...(patternMeta?.description && { description: patternMeta.description }), | ||
| // schemaMappings carries the dataset's correlation/field-mapping config. | ||
| ...(patternMeta?.schemaMappings && { schemaMappings: patternMeta.schemaMappings }), |
There was a problem hiding this comment.
Worth confirming the intent here: this line only ever fires for fetchDefaultDataset. The other caller, fetchIndexPatterns just below (line ~118), doesn't request schemaMappings in its fields projection and doesn't set it in meta, so the dataset-selector path never populates it — and dataset_select.tsx:450 documents that as deliberate:
schemaMappingsis intentionally not carried here — the list/tooltip don't use it, and the consumers that do (e.g. trace-log correlation) re-resolve the dataset by id themselves.
Two things follow:
- The PR description says re-selecting from the picker "runs a different, more complete conversion path." For
schemaMappingsit's the opposite — the picker path is the one that omits it. - Every reader I could find (
url_builder.ts:31,119,ppl_request_logs.tsx:38,correlation_service.fetchLogDataset) readsschemaMappingsoff the re-resolved log dataset, not off the query-state dataset. So persisting it into_qlengthens every Explore URL without a demonstrated consumer.
Does the reported traces failure actually reproduce with just dataSource + signalType restored? If so, dropping schemaMappings from the persisted shape would keep URLs smaller and stay consistent with the note above.
There was a problem hiding this comment.
Agreed, and thanks for the detail. Dropped schemaMappings from the persisted path entirely — removed it from extractSerializableDataset, this toDataset line, and the fetchDefaultDataset meta. No query-state consumer reads it (correlation/logs re-resolve the referenced dataset by id) and the picker omits it deliberately, so this keeps URLs smaller and consistent. dataSource + signalType are what the traces load needs. Updated the PR description to drop the now-incorrect "picker runs a more complete path" framing for schemaMappings. (commit 9c7e2b7)
| // Carry signalType and schemaMappings through meta so toDataset produces a fully | ||
| // hydrated default dataset (signal-type routing + correlation config). | ||
| ...(indexPattern.signalType && { signalType: indexPattern.signalType }), | ||
| ...(indexPattern.schemaMappings && { schemaMappings: indexPattern.schemaMappings }), |
There was a problem hiding this comment.
Since you're already adding meta keys here — datasetType is missing, and toDataset reads the dataset type from meta, not from the DataStructure:
type: patternMeta?.datasetType || DEFAULT_DATA.SET_TYPES.INDEX_PATTERN,This builds the structure with type: actualType but no meta.datasetType, so a non-standard index pattern (e.g. a rollup) gets flattened back to INDEX_PATTERN in the default dataset. fetchIndexPatterns does carry it (datasetType: savedObject.attributes.type), so this is the same divergence-between-converters problem the PR is fixing, one field over:
...(actualType && { datasetType: actualType }),There was a problem hiding this comment.
Added datasetType: actualType to the meta so a non-standard index pattern (e.g. a rollup) is no longer flattened to INDEX_PATTERN in the default dataset — matches what fetchIndexPatterns already carries. (commit 9c7e2b7)
| expect(mockSavedObjectsClient.find).toHaveBeenCalledWith({ | ||
| type: 'correlations', | ||
| fields: ['correlations', 'references'], | ||
| fields: ['entities'], |
There was a problem hiding this comment.
This pins the argument, but it can't catch the bug class it's covering: mockSavedObjectsClient.find is a jest.fn() that returns its fixture regardless of what fields asks for, so the production code would pass this test whether or not entities is requested.
Relatedly, the fixture a few lines up still feeds a shape production can no longer receive:
attributes: {
correlations: { entities: [{ id: 'test-dataset-id' }] },
},With fields: ['entities'] that attribute never comes back (and per the mapping it's never written in the first place). The test passes on the references match instead, so the fixture is now actively misleading about what the code does.
Suggest reshaping it to the real saved-object shape — attributes.entities: [{ tracesDataset: ... }, { logsDataset: ... }] plus references — which would also give checkCorrelationsForLogs a case that actually exercises the traces↔logs branch this PR is fixing.
There was a problem hiding this comment.
Right — reshaped the fixture to the real attributes.entities: [{ tracesDataset }, { logsDataset }] + references shape and added a pass-through test. Discovery is now server-side via hasReference (see that thread), and the traces↔logs resolution branch is exercised by the checkCorrelationsForLogs cases. (commit 9c7e2b7)
| type: datasetType, | ||
| timeFieldName: this.timeFieldName, | ||
| // signalType (traces/metrics/logs) drives flavor routing in consumers like Explore. | ||
| signalType: this.signalType, |
There was a problem hiding this comment.
p1: the dataSource fix is load-bearing on signalType, and the fall-through path is unproven
Verified this fixes the reported symptom, but indirectly: no converter in this diff adds dataSource. It works because populating signalType lets resolveDataset (redux_persistence.ts:279) return the already-hydrated existing/default dataset instead of falling through to fetchFirstAvailableDataset, which builds datasets via toDataset([pattern]) over fetch(services, []) (empty ancestry, so pattern.parent is undefined and no dataSource is set) and is unchanged here. So on any genuine fresh load where resolution does fall through, the _q.dataset can still come back without dataSource - the original "no cluster to query" failure. The added test only covers copying an already-present dataSource, so it doesn't prove the customer symptom is gone on the fetchFirstAvailableDataset path. Could you either add a test asserting the serialized dataset carries dataSource on a real fresh load (where fetchFirstAvailableDataset is the resolver), or hydrate dataSource in that path too?
There was a problem hiding this comment.
Verified: the fall-through path does hydrate dataSource. fetchFirstAvailableDataset builds children via fetch() → fetchIndexPatterns, which sets each pattern's parent from its own data-source reference (the [] is the ancestry path arg, not the children source), and the real toDataset([pattern]) turns parent into dataset.dataSource. Both halves are already covered in index_pattern_type.test.ts (fetch→parent, toDataset→dataSource); I added an end-to-end test in redux_persistence.test.ts that drives fetchFirstAvailableDataset with the real toDataset and asserts the resolved + serialized dataset carries dataSource on the fresh-load path. So no separate hydration is needed — a genuinely data-source-less pattern is a local/default-cluster index pattern that correctly needs none. (commit d8a6044)
…ss dataset reducer, drop dead paths Signed-off-by: Adam Tackett <tackadam@amazon.com>
|
Persistent review updated to latest commit 9c7e2b8 |
PR Code Suggestions ✨Latest suggestions up to cb5faea Explore these optional code suggestions:
Previous suggestionsSuggestions up to commit d8a6044
Suggestions up to commit 9c7e2b8
|
…race dataset Signed-off-by: Adam Tackett <tackadam@amazon.com>
|
Persistent review updated to latest commit d8a6044 |
|
Cross-reference: this PR interacts with #12743 ( #12743 removes the route this PR's fresh-load fix travels. const existingDataset = preferredDataset || queryStringQuery?.dataset || defaultQuery?.dataset;#12743 makes This PR landing first is the safer order. #12743 adds an Explore Logs init that pushes the default dataset into the shared query; with Neither PR has a test covering the interaction. |
…ate PR Signed-off-by: Adam Tackett <tackadam@amazon.com>
|
Persistent review updated to latest commit cb5faea |
|
Heads up on scope: I've adjusted this PR to focus only on dataset-field hydration for the Explore views (Logs / Traces / Metrics), and removed the trace-to-log correlation-resolution change that was originally included here. The reported correlation failure turned out to have a distinct root cause — a PPL As a result, some earlier review threads on |
Description
Fixes incomplete dataset hydration on first load of the Explore views (
app/explore/logs/traces/metrics).On a fresh navigation, the dataset serialized into the
_qURL parameter was missingdataSource,displayName, andsignalType. With nodataSourcethere is no cluster to query and the view fails until the user manually re-selects the dataset. The underlying cause is that several independent IndexPattern→Dataset converters each drop a different subset of fields. This PR makes the relevant converters consistent and gives Explore a single lossless reducer:DataView.toDataset()now includessignalType.DataViewsService.convertToDataset()non-DataViewfallback now includessignalType, matching thetoDataset()branch.fetchDefaultDataset()threadssignalTypeanddatasetTypethroughDataStructuremeta, so the default dataset is fully hydrated (and a non-standard type such as a rollup is no longer flattened toINDEX_PATTERN). No extra saved-object lookups are introduced — these are read from the index pattern that is already fetched.extractSerializableDataset()helper that copies dataset hydration fields explicitly (so class methods can't leak into serialized state) and dropsundefinedkeys (so a freshly-extracted dataset compares equal to one read back from_q). It replaces the two hand-rolled "minimal dataset" reducers inredux_persistence.tsand the in-context visualization editor's copy.schemaMappingsis intentionally not persisted into_q: no query-state consumer reads it (consumers re-resolve the referenced dataset by id), and the dataset-selector list omits it by design, so persisting it would only lengthen the URL.Scope
getPreloadedQueryState→extractSerializableDataset, so all are covered.data.query.queryStringstate, so it benefits automatically from the converter fixes above — no parallel change needed.awaitonconvertToDataset()in the traces execution path (trace_query_actions.ts).This is compatible with the recent load-time optimizations: the lightweight
fetchIndexPatternsprojection is unchanged, andsignalTypecontinues to be populated bytoDatasetrather than per-datasetDataViewfetches.Issues Resolved
N/A
Screenshot
N/A — behavior fix on data hydration; no visual changes.
Testing the changes
yarn test:jestfor the affected suites (data views, dataset service, index pattern type, Explore redux persistence, in-context vis editor, trace query actions) — all pass. Added coverage forextractSerializableDataset(field preservation, undefined-stripping, no class-method leakage) and the fresh-load fallback carryingdataSource.yarn typecheckpasses.Manual: open an Explore view (Logs/Traces/Metrics) in a fresh session and confirm it loads against the correct data source without re-selecting the dataset.
Check List
yarn test:jestyarn test:jest_integration