Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Vis Builder] Bug fixes for datasource picker and auto time interval #2632

Merged
merged 6 commits into from
Oct 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
* [Vis Builder] Fixes visualization shift when editing agg ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
* [Vis Builder] Renames "Histogram" to "Bar" in vis type picker ([2401](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2401))
* [Vis Builder] Update vislib params and misc fixes ([2610](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2610))
* [Vis Builder] Bug fixes for datasource picker and auto time interval ([2632](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2632))
* [MD] Add data source param to low-level search call in Discover ([#2431](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2431))
* [Multi DataSource] Skip data source view in index pattern step when pick default ([#2574](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2574))
* [Multi DataSource] Address UX comments on Edit Data source page ([#2629](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2629))
Expand Down
5 changes: 2 additions & 3 deletions src/core/public/saved_objects/simple_saved_object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ export class SimpleSavedObject<T = unknown> {
error,
references,
migrationVersion,
// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is simply to remove the eslint exception

updated_at,
updated_at: updateAt,
}: SavedObjectType<T>
) {
this.id = id;
Expand All @@ -73,7 +72,7 @@ export class SimpleSavedObject<T = unknown> {
this.references = references || [];
this._version = version;
this.migrationVersion = migrationVersion;
this.updated_at = updated_at;
this.updated_at = updateAt;
if (error) {
this.error = error;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,17 @@ import { cloneDeep } from 'lodash';
import { BucketAggType, IndexPatternField, propFilter } from '../../../../../../data/common';
import { Schema } from '../../../../../../vis_default_editor/public';
import { COUNT_FIELD, FieldDragDataType } from '../../../utils/drag_drop/types';
import { useTypedDispatch, useTypedSelector } from '../../../utils/state_management';
import { useTypedDispatch } from '../../../utils/state_management';
import { DropboxDisplay, DropboxProps } from '../dropbox';
import { useDrop } from '../../../utils/drag_drop';
import {
editDraftAgg,
reorderAgg,
updateAggConfigParams,
} from '../../../utils/state_management/visualization_slice';
import { useIndexPatterns } from '../../../utils/use/use_index_pattern';
import { useOpenSearchDashboards } from '../../../../../../opensearch_dashboards_react/public';
import { VisBuilderServices } from '../../../../types';
import { useAggs } from '../../../utils/use';

const filterByName = propFilter('name');
const filterByType = propFilter('type');
Expand All @@ -30,36 +30,34 @@ export interface UseDropboxProps extends Pick<DropboxProps, 'id' | 'label'> {
export const useDropbox = (props: UseDropboxProps): DropboxProps => {
const { id: dropboxId, label, schema } = props;
const [validAggTypes, setValidAggTypes] = useState<string[]>([]);
const { aggConfigs, indexPattern, aggs, timeRange } = useAggs();
const dispatch = useTypedDispatch();
const indexPattern = useIndexPatterns().selected;
const {
services: {
data: {
search: { aggs: aggService },
},
},
} = useOpenSearchDashboards<VisBuilderServices>();
const aggConfigParams = useTypedSelector(
(state) => state.visualization.activeVisualization?.aggConfigParams
);

const aggConfigs = useMemo(() => {
return indexPattern && aggService.createAggConfigs(indexPattern, cloneDeep(aggConfigParams));
}, [aggConfigParams, aggService, indexPattern]);

const aggs = useMemo(() => aggConfigs?.aggs ?? [], [aggConfigs?.aggs]);

const dropboxAggs = aggs.filter((agg) => agg.schema === schema.name);
const dropboxAggs = useMemo(() => aggs.filter((agg) => agg.schema === schema.name), [
aggs,
schema.name,
]);

const displayFields: DropboxDisplay[] = useMemo(
() =>
dropboxAggs?.map(
(agg): DropboxDisplay => ({
id: agg.id,
label: agg.makeLabel(),
})
) || [],
[dropboxAggs]
(agg): DropboxDisplay => {
// For timeseries aggregations that have timeinterval set as auto, the current timerange is required to calculate the label accurately
agg.aggConfigs.setTimeRange(timeRange);
return {
id: agg.id,
label: agg.makeLabel(),
};
}
) ?? [],
[dropboxAggs, timeRange]
);

// Event handlers for each dropbox action type
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export const Option: FC<Props> = ({ title, children, initialIsOpen = false }) =>
buttonContent={title}
className="vbOption"
initialIsOpen={initialIsOpen}
data-test-subj={`vbOption-${title.replace(/\s+/g, '-')}`}
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved
>
<EuiSpacer size="s" />
<EuiPanel color="subdued" className="vbOption__panel">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const Workspace: FC = ({ children }) => {
useEffect(() => {
async function loadExpression() {
const schemas = ui.containerConfig.data.schemas;
const [valid, errorMsg] = validateSchemaState(schemas, rootState);
const [valid, errorMsg] = validateSchemaState(schemas, rootState.visualization);

if (!valid) {
if (errorMsg) {
Expand All @@ -50,12 +50,13 @@ export const Workspace: FC = ({ children }) => {
setExpression(undefined);
return;
}
const exp = await toExpression(rootState);

const exp = await toExpression(rootState, searchContext);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now passing search context too since certain visualizations need the search context data to correctly configure the expression

setExpression(exp);
}

loadExpression();
}, [rootState, toExpression, toasts, ui.containerConfig.data.schemas]);
}, [rootState, toExpression, toasts, ui.containerConfig.data.schemas, searchContext]);

useLayoutEffect(() => {
const subscription = data.query.state$.subscribe(({ state }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export const getPreloadedStore = async (services: VisBuilderServices) => {

// Infer the `RootState` and `AppDispatch` types from the store itself
export type RootState = ReturnType<typeof rootReducer>;
export type RenderState = Omit<RootState, 'metadata'>; // Remaining state after auxillary states are removed
type Store = ReturnType<typeof configurePreloadedStore>;
export type AppDispatch = Store['dispatch'];

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export const slice = createSlice({
setIndexPattern: (state, action: PayloadAction<string>) => {
state.indexPattern = action.payload;
state.activeVisualization!.aggConfigParams = [];
state.activeVisualization!.draftAgg = undefined;
},
setSearchField: (state, action: PayloadAction<string>) => {
state.searchField = action.payload;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

export { useAggs } from './use_aggs';
export { useVisualizationType } from './use_visualization_type';
export { useIndexPatterns } from './use_index_pattern';
export { useSavedVisBuilderVis } from './use_saved_vis_builder_vis';
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { cloneDeep } from 'lodash';
import { useLayoutEffect, useMemo, useState } from 'react';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import { VisBuilderServices } from '../../../types';
import { useTypedSelector, useTypedDispatch } from '../state_management';
import { useIndexPatterns } from './use_index_pattern';

/**
* Returns common agg parameters from the store and app context
* @returns { indexPattern, aggConfigs, aggs, timeRange }
*/
export const useAggs = () => {
const {
services: {
data: {
search: { aggs: aggService },
query: {
timefilter: { timefilter },
},
},
},
} = useOpenSearchDashboards<VisBuilderServices>();
const indexPattern = useIndexPatterns().selected;
const [timeRange, setTimeRange] = useState(timefilter.getTime());
const aggConfigParams = useTypedSelector(
(state) => state.visualization.activeVisualization?.aggConfigParams
);
const dispatch = useTypedDispatch();

const aggConfigs = useMemo(() => {
const configs =
indexPattern && aggService.createAggConfigs(indexPattern, cloneDeep(aggConfigParams));
return configs;
}, [aggConfigParams, aggService, indexPattern]);

useLayoutEffect(() => {
const subscription = timefilter.getTimeUpdate$().subscribe(() => {
setTimeRange(timefilter.getTime());
});

return () => {
subscription.unsubscribe();
};
}, [dispatch, timefilter]);

return {
indexPattern,
aggConfigs,
aggs: aggConfigs?.aggs ?? [],
timeRange,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,19 @@

import { countBy } from 'lodash';
import { Schemas } from '../../../../vis_default_editor/public';
import { RootState } from './state_management';
import { VisualizationState } from './state_management';

export const validateSchemaState = (schemas: Schemas, state: RootState): [boolean, string?] => {
const activeViz = state.visualization.activeVisualization;
/**
* Validate if the visualization state fits the vis type schema criteria
* @param schemas Visualization type config Schema objects
* @param state visualization state
* @returns [Validity, 'Message']
*/
export const validateSchemaState = (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified this since we can only reliably validate on he visualization state.

schemas: Schemas,
state: VisualizationState
): [boolean, string?] => {
const activeViz = state.activeVisualization;
const vizName = activeViz?.name;
const aggs = activeViz?.aggConfigParams;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
import { validateSchemaState } from '../application/utils/validate_schema_state';
import { getExpressionLoader, getTypeService } from '../plugin_services';
import { PersistedState } from '../../../visualizations/public';
import { RenderState, VisualizationState } from '../application/utils/state_management';

// Apparently this needs to match the saved object type for the clone and replace panel actions to work
export const VISBUILDER_EMBEDDABLE = VISBUILDER_SAVED_OBJECT;
Expand Down Expand Up @@ -121,24 +122,24 @@ export class VisBuilderEmbeddable extends Embeddable<SavedObjectEmbeddableInput,
const { visualization, style } = this.serializedState;

const vizStateWithoutIndex = JSON.parse(visualization);
const visualizationState = {
const visualizationState: VisualizationState = {
searchField: vizStateWithoutIndex.searchField,
activeVisualization: vizStateWithoutIndex.activeVisualization,
indexPattern: this.savedVisBuilder?.searchSourceFields?.index,
};
const rootState = {
const renderState: RenderState = {
visualization: visualizationState,
style: JSON.parse(style),
};
const visualizationName = rootState.visualization?.activeVisualization?.name ?? '';
const visualizationName = renderState.visualization?.activeVisualization?.name ?? '';
const visualizationType = getTypeService().get(visualizationName);
if (!visualizationType) {
this.onContainerError(new Error(`Invalid visualization type ${visualizationName}`));
return;
}
const { toExpression, ui } = visualizationType;
const schemas = ui.containerConfig.data.schemas;
const [valid, errorMsg] = validateSchemaState(schemas, rootState);
const [valid, errorMsg] = validateSchemaState(schemas, visualizationState);

if (!valid) {
if (errorMsg) {
Expand All @@ -147,7 +148,11 @@ export class VisBuilderEmbeddable extends Embeddable<SavedObjectEmbeddableInput,
}
} else {
// TODO: handle error in Expression creation
const exp = await toExpression(rootState);
const exp = await toExpression(renderState, {
filters: this.filters,
query: this.query,
timeRange: this.timeRange,
});
return exp;
}
};
Expand Down Expand Up @@ -268,12 +273,15 @@ export class VisBuilderEmbeddable extends Embeddable<SavedObjectEmbeddableInput,
// Check if rootState has changed
if (!isEqual(this.getSerializedState(), this.serializedState)) {
this.serializedState = this.getSerializedState();
this.expression = (await this.getExpression()) ?? '';
dirty = true;
}

if (this.handler && dirty) {
this.updateHandler();
if (dirty) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to recalculate the expression if the context changes too.

this.expression = (await this.getExpression()) ?? '';

if (this.handler) {
this.updateHandler();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
*/
import { ReactElement } from 'react';
import { IconType } from '@elastic/eui';
import { RootState } from '../../application/utils/state_management';
import { RenderState } from '../../application/utils/state_management';
import { Schemas } from '../../../../vis_default_editor/public';
import { IExpressionLoaderParams } from '../../../../expressions/public';

export interface DataTabConfig {
schemas: Schemas;
Expand All @@ -28,5 +29,8 @@ export interface VisualizationTypeOptions<T = any> {
style: StyleTabConfig<T>;
};
};
readonly toExpression: (state: RootState) => Promise<string | undefined>;
readonly toExpression: (
state: RenderState,
searchContext: IExpressionLoaderParams['searchContext']
) => Promise<string | undefined>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
* SPDX-License-Identifier: Apache-2.0
*/
import { IconType } from '@elastic/eui';
import { RootState } from '../../application/utils/state_management';
import { IExpressionLoaderParams } from '../../../../expressions/public';
import { RenderState } from '../../application/utils/state_management';
import { VisualizationTypeOptions } from './types';

type IVisualizationType = VisualizationTypeOptions;
Expand All @@ -15,7 +16,10 @@ export class VisualizationType implements IVisualizationType {
public readonly icon: IconType;
public readonly stage: 'experimental' | 'production';
public readonly ui: IVisualizationType['ui'];
public readonly toExpression: (state: RootState) => Promise<string | undefined>;
public readonly toExpression: (
state: RenderState,
searchContext: IExpressionLoaderParams['searchContext']
) => Promise<string | undefined>;

constructor(options: VisualizationTypeOptions) {
this.name = options.name;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { SchemaConfig } from '../../../../visualizations/public';
import { MetricVisExpressionFunctionDefinition } from '../../../../vis_type_metric/public';
import { AggConfigs, IAggConfig } from '../../../../data/common';
import { buildExpression, buildExpressionFunction } from '../../../../expressions/public';
import { RootState } from '../../application/utils/state_management';
import { RenderState } from '../../application/utils/state_management';
import { MetricOptionsDefaults } from './metric_viz_type';
import { getAggExpressionFunctions } from '../common/expression_helpers';

Expand Down Expand Up @@ -82,7 +82,7 @@ const getVisSchemas = (aggConfigs: AggConfigs): any => {
return schemas;
};

export interface MetricRootState extends RootState {
export interface MetricRootState extends RenderState {
style: MetricOptionsDefaults;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,26 @@
*/

import { buildVislibDimensions } from '../../../../../visualizations/public';
import { buildExpression, buildExpressionFunction } from '../../../../../expressions/public';
import {
buildExpression,
buildExpressionFunction,
IExpressionLoaderParams,
} from '../../../../../expressions/public';
import { AreaOptionsDefaults } from './area_vis_type';
import { getAggExpressionFunctions } from '../../common/expression_helpers';
import { VislibRootState, getValueAxes, getPipelineParams } from '../common';
import { createVis } from '../common/create_vis';

export const toExpression = async ({
style: styleState,
visualization,
}: VislibRootState<AreaOptionsDefaults>) => {
export const toExpression = async (
{ style: styleState, visualization }: VislibRootState<AreaOptionsDefaults>,
searchContext: IExpressionLoaderParams['searchContext']
) => {
const { aggConfigs, expressionFns, indexPattern } = await getAggExpressionFunctions(
visualization
);
const { addLegend, addTooltip, legendPosition, type } = styleState;

const vis = await createVis(type, aggConfigs, indexPattern);
const vis = await createVis(type, aggConfigs, indexPattern, searchContext?.timeRange);

const params = getPipelineParams();
const dimensions = await buildVislibDimensions(vis, params);
Expand Down
Loading