[Backport 2.x] Enable plugins to augment visualizations with additional data and context (#4361) #4517
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…text Signed-off-by: Tyler Ohlsen <[email protected]>
ohltyler
requested review from
ananzh,
kavilla,
seanneumann,
AMoo-Miki,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
kristenTian,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub and
BSFishy
as code owners
Codecov Report
@@ Coverage Diff @@
## 2.x #4517 +/- ##
==========================================
+ Coverage 66.40% 66.46% +0.06%
==========================================
Files 3245 3287 +42
Lines 62451 63286 +835
Branches 9712 9844 +132
==========================================
+ Hits 41470 42063 +593
- Misses 18624 18844 +220
- Partials 2357 2379 +22
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
The build workflow that is failing is unrelated to this change and is also seen in The codecov failure is fixed in follow up PR #4516 that will bump the coverage. |
lezzago
previously approved these changes
Jul 7, 2023
AMoo-Miki
reviewed
Jul 7, 2023
Co-authored-by: Miki <[email protected]> Signed-off-by: Tyler Ohlsen <[email protected]>
lezzago
approved these changes
Jul 7, 2023
AMoo-Miki
reviewed
Jul 7, 2023
|
|
||
| Saved objects that have relationships to index patterns are saved using the [`kibanaSavedObjectMeta`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/saved_objects/public/saved_object/helpers/serialize_saved_object.ts#L59) attribute and the [`references`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/saved_objects/public/saved_object/helpers/serialize_saved_object.ts#L60) array structure. Functions from the data plugin are used by the saved object plugin to manage this index pattern relationship. | ||
| Saved objects can persist parent/child relationships to other saved objects via `references`. These relationships can be viewed on the UI in the [saved objects management plugin](src/core/server/saved_objects_management/README.md). Relationships can be useful to combine existing saved objects to produce new ones, such as using an index pattern as the source for a visualization, or a dashboard consisting of many visualizations. |
There was a problem hiding this comment.
Maybe in a follow-up PR:
Suggested change
| Saved objects can persist parent/child relationships to other saved objects via `references`. These relationships can be viewed on the UI in the [saved objects management plugin](src/core/server/saved_objects_management/README.md). Relationships can be useful to combine existing saved objects to produce new ones, such as using an index pattern as the source for a visualization, or a dashboard consisting of many visualizations. | |
| Any parent/child relationships among saved objects can persist using their `references` property. These relationships can be viewed on the UI in the [saved objects management plugin](src/core/server/saved_objects_management/README.md). Relationships can be useful to combine existing saved objects to produce new ones, such as using an index pattern as the source for a visualization, or a dashboard consisting of many visualizations. |
There was a problem hiding this comment.
Sure - I am currently starting a followup documentation PR anyways so I can include these changes there. thanks!
AMoo-Miki
reviewed
Jul 7, 2023
| "type" : "visualization", | ||
| "references" : [ | ||
| { | ||
| "name" : "kibanaSavedObjectMeta.searchSourceJSON.index", |
There was a problem hiding this comment.
Is kibanaSavedObjectMeta a keyword? If not, can we use our own name?
There was a problem hiding this comment.
this is just what exists currently for visualizations. it's a saved object attribute for the visualization saved obj type. i haven't changed anything here
AMoo-Miki
reviewed
Jul 7, 2023
| ### Others | ||
|
|
||
| If a saved object type wishes to have additional custom functionalities when extracting/injecting references, or after OpenSearch's response, it can define functions in the class constructor when extending the `SavedObjectClass`. For example, visualization plugin's [`SavedVis`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts#L91) class has additional `extractReferences`, `injectReferences` and `afterOpenSearchResp` functions defined in [`_saved_vis.ts`](../visualizations/public/saved_visualizations/_saved_vis.ts). | ||
| Steps need to be done on both the public/client-side & the server-side for creating a new saved object type. |
There was a problem hiding this comment.
Suggested change
| Steps need to be done on both the public/client-side & the server-side for creating a new saved object type. | |
| Certain actions need to be completed on both, the client-side and the server-side, for creating a new saved object type. |
AMoo-Miki
reviewed
Jul 7, 2023
|
|
||
| Client-side: | ||
|
|
||
| 1. Define a class that extends `SavedObjectClass`. This is where custom functionalities, such as extracting/injecting references, or overriding `afterOpenSearchResp` can be set in the constructor. For example, visualization plugin's [`SavedVis`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts#L91) class has additional `extractReferences`, `injectReferences` and `afterOpenSearchResp` functions defined in [`_saved_vis.ts`](../visualizations/public/saved_visualizations/_saved_vis.ts), and set in the `SavedVis` constructor. |
There was a problem hiding this comment.
nit:
Suggested change
| 1. Define a class that extends `SavedObjectClass`. This is where custom functionalities, such as extracting/injecting references, or overriding `afterOpenSearchResp` can be set in the constructor. For example, visualization plugin's [`SavedVis`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts#L91) class has additional `extractReferences`, `injectReferences` and `afterOpenSearchResp` functions defined in [`_saved_vis.ts`](../visualizations/public/saved_visualizations/_saved_vis.ts), and set in the `SavedVis` constructor. | |
| 1. Define a class that extends `SavedObjectClass`. This is where custom functionalities, such as extracting/injecting references or overriding `afterOpenSearchResp`, can be set in the constructor. For example, visualization plugin's [`SavedVis`](https://github.com/opensearch-project/OpenSearch-Dashboards/blob/4a06f5a6fe404a65b11775d292afaff4b8677c33/src/plugins/visualizations/public/saved_visualizations/_saved_vis.ts#L91) class has additional `extractReferences`, `injectReferences`, and `afterOpenSearchResp` functions defined in [`_saved_vis.ts`](../visualizations/public/saved_visualizations/_saved_vis.ts) and set in the `SavedVis` constructor. |
AMoo-Miki
reviewed
Jul 7, 2023
| return (savedVis as any) as SavedObject; | ||
| }, | ||
| ``` | ||
|
|
||
| 2. Optionally create a loader class that extends `SavedObjectLoader`. This can be useful for performing default CRUD operations on this particular saved object type, as well as overriding default utility functions like `find`. For example, the `visualization` saved object overrides `mapHitSource` (used in `find` & `findAll`) to do additional checking on the returned source object, such as if the returned type is valid: |
There was a problem hiding this comment.
nit:
Suggested change
| 2. Optionally create a loader class that extends `SavedObjectLoader`. This can be useful for performing default CRUD operations on this particular saved object type, as well as overriding default utility functions like `find`. For example, the `visualization` saved object overrides `mapHitSource` (used in `find` & `findAll`) to do additional checking on the returned source object, such as if the returned type is valid: | |
| 2. Optionally, create a loader class that extends `SavedObjectLoader`. This can be useful for performing default CRUD operations on this particular saved object type and overriding default utility functions like `find`. For example, the `visualization` saved object overrides `mapHitSource` (used in `find` and `findAll`) to perform additional checks on the returned source object, such as checking if the returned type is valid: |
|
@ohltyler All of Miki's comments are about doc fixes and additional tests, both of which you are working on i believe in a follow up. |
AMoo-Miki
reviewed
Jul 7, 2023
|
|
||
| public getDisplayName() { | ||
| return i18n.translate('dashboard.actions.deleteSavedObject.name', { | ||
| defaultMessage: 'Clean up augment-vis saved objects associated to a deleted vis', |
There was a problem hiding this comment.
Can we change this to non-shortened text?
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+51
to
+52
| .filter((augmentVisObj) => augmentVisObj.visId === savedObjectId) | ||
| .map((augmentVisObj) => augmentVisObj.id as string); |
There was a problem hiding this comment.
Should we be concerned that the tests didn't cover these; being "delete" worries me.
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+216
to
+219
| type: get(resource, 'type', 'test-resource-type'), | ||
| id: get(resource, 'id', 'test-resource-id'), | ||
| name: get(resource, 'name', 'test-resource-name'), | ||
| urlPath: get(resource, 'urlPath', 'test-resource-url-path'), |
There was a problem hiding this comment.
These have to be replaced; there is no need for get here.
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+51
to
+55
| description: get(opts, 'description', ''), | ||
| originPlugin: get(opts, 'originPlugin', ''), | ||
| pluginResource: get(opts, 'pluginResource', {}), | ||
| visId: get(opts, 'visId', ''), | ||
| visLayerExpressionFn: get(opts, 'visLayerExpressionFn', {}), |
There was a problem hiding this comment.
Again, there is no need to use get here.
AMoo-Miki
reviewed
Jul 7, 2023
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| import { get, isEmpty } from 'lodash'; |
There was a problem hiding this comment.
Let;s not introduce more uses of lodash; we are actively trying to get rid of it.
AMoo-Miki
reviewed
Jul 7, 2023
| if (updatedAttributes.visId) { | ||
| updatedReferences.push({ | ||
| name: VIS_REFERENCE_NAME, | ||
| type: 'visualization', |
There was a problem hiding this comment.
Is the term visualization a constant somewhere we can import?
AMoo-Miki
reviewed
Jul 7, 2023
| uiSettings?: IUiSettingsClient | ||
| ): Promise<any> => { | ||
| // Using optional services provided, or the built-in services from this plugin | ||
| const loader = savedObjLoader !== undefined ? savedObjLoader : getSavedAugmentVisLoader(); |
There was a problem hiding this comment.
Wouldn't this be safer? What if savedObjLoader is null?
Suggested change
| const loader = savedObjLoader !== undefined ? savedObjLoader : getSavedAugmentVisLoader(); | |
| const loader = savedObjLoader || getSavedAugmentVisLoader(); |
AMoo-Miki
reviewed
Jul 7, 2023
| export const pluginResourceDeleteTrigger: Trigger<'PLUGIN_RESOURCE_DELETE_TRIGGER'> = { | ||
| id: PLUGIN_RESOURCE_DELETE_TRIGGER, | ||
| title: i18n.translate('visAugmenter.triggers.pluginResourceDeleteTitle', { | ||
| defaultMessage: 'Plugin resource delete', |
There was a problem hiding this comment.
As a trigger, this title is wrong.
AMoo-Miki
reviewed
Jul 7, 2023
| defaultMessage: 'Plugin resource delete', | ||
| }), | ||
| description: i18n.translate('visAugmenter.triggers.pluginResourceDeleteDescription', { | ||
| defaultMessage: 'Delete augment-vis saved objs associated to the deleted plugin resource', |
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+30
to
+34
| vis.data?.aggs !== undefined && | ||
| vis.data.aggs?.byTypeName('date_histogram').length === 1 && | ||
| vis.params.categoryAxes.length === 1 && | ||
| vis.params.categoryAxes[0].position === 'bottom' && | ||
| vis.data.aggs?.bySchemaName('segment').length > 0; |
There was a problem hiding this comment.
Suggested change
| vis.data?.aggs !== undefined && | |
| vis.data.aggs?.byTypeName('date_histogram').length === 1 && | |
| vis.params.categoryAxes.length === 1 && | |
| vis.params.categoryAxes[0].position === 'bottom' && | |
| vis.data.aggs?.bySchemaName('segment').length > 0; | |
| vis.data?.aggs?.byTypeName && | |
| vis.data.aggs.byTypeName('date_histogram').length === 1 && | |
| vis.params.categoryAxes.length === 1 && | |
| vis.params.categoryAxes[0].position === 'bottom' && | |
| vis.data.aggs.bySchemaName('segment').length > 0; |
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+38
to
+41
| const hasCorrectAggregationCount = | ||
| vis.data?.aggs !== undefined && | ||
| vis.data.aggs?.bySchemaName('metric').length > 0 && | ||
| vis.data.aggs?.bySchemaName('metric').length === vis.data.aggs?.aggs.length - 1; |
There was a problem hiding this comment.
Suggested change
| const hasCorrectAggregationCount = | |
| vis.data?.aggs !== undefined && | |
| vis.data.aggs?.bySchemaName('metric').length > 0 && | |
| vis.data.aggs?.bySchemaName('metric').length === vis.data.aggs?.aggs.length - 1; | |
| const visAggsMetricLength = vis.data.aggs?.bySchemaName?.('metric')?.length || 0; | |
| const hasCorrectAggregationCount = | |
| visAggsMetricLength > 0 && | |
| visAggsMetricLength === vis.data.aggs?.aggs.length - 1; |
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+43
to
+47
| vis.params?.seriesParams !== undefined && | ||
| vis.params?.seriesParams?.every( | ||
| (seriesParam: { type: string }) => seriesParam.type === 'line' | ||
| ) && | ||
| vis.params?.type === 'line'; |
There was a problem hiding this comment.
Suggested change
| vis.params?.seriesParams !== undefined && | |
| vis.params?.seriesParams?.every( | |
| (seriesParam: { type: string }) => seriesParam.type === 'line' | |
| ) && | |
| vis.params?.type === 'line'; | |
| vis.params?.type === 'line' && | |
| vis.params.seriesParams?.every?.( | |
| (seriesParam: { type: string }) => seriesParam.type === 'line' | |
| ); |
AMoo-Miki
reviewed
Jul 7, 2023
| // Checks if the augmentation setting is enabled | ||
| const config = uiSettingsClient ?? getUISettings(); | ||
| const isAugmentationEnabled = config.get(PLUGIN_AUGMENTATION_ENABLE_SETTING); | ||
| return isAugmentationEnabled && hasValidXaxis && hasCorrectAggregationCount && hasOnlyLineSeries; |
There was a problem hiding this comment.
Instead of doing all those things above and finally responding, we should return false after each one is assessed; doing it all at once is wasteful.
AMoo-Miki
reviewed
Jul 7, 2023
| uiSettings?: IUiSettingsClient | undefined | ||
| ): Promise<ISavedAugmentVis[]> => { | ||
| // Using optional services provided, or the built-in services from this plugin | ||
| const config = uiSettings !== undefined ? uiSettings : getUISettings(); |
There was a problem hiding this comment.
I see
const config = uiSettingsClient ?? getUISettings();
and
const config = uiSettings !== undefined ? uiSettings : getUISettings();
used in different places. While it would be a lot nicer to have just one of them, I would argue both are wrong and we should do
const config = uiSettings || getUISettings();
AMoo-Miki
reviewed
Jul 7, 2023
| ): Promise<ISavedAugmentVis[]> => { | ||
| try { | ||
| const resp = await loader?.findAll(); | ||
| return (get(resp, 'hits', []) as any[]) as ISavedAugmentVis[]; |
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+171
to
+178
| const objIdsToDelete = [] as string[]; | ||
| staleVisLayers.forEach((staleVisLayer) => { | ||
| // Match the VisLayer to its origin saved obj to extract the to-be-deleted saved obj ID | ||
| const deletedPluginResourceId = staleVisLayer.pluginResource.id; | ||
| const savedObjId = augmentVisSavedObjs.find( | ||
| (savedObj) => savedObj.pluginResource.id === deletedPluginResourceId | ||
| )?.id; | ||
| if (savedObjId !== undefined) objIdsToDelete.push(savedObjId); |
There was a problem hiding this comment.
This would be faster if we throw the augmentVisSavedObjs[].pluginResource.id into a set or object than search an array for each.
AMoo-Miki
reviewed
Jul 7, 2023
| import { ErrorFlyoutBody } from './error_flyout_body'; | ||
|
|
||
| describe('<ErrorFlyoutBody/>', () => { | ||
| const errorMsg = 'oh no an error!'; |
There was a problem hiding this comment.
This has to change even if it is just a test.
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+22
to
+23
| const name = get(props, 'item.visLayer.pluginResource.name', ''); | ||
| const urlPath = get(props, 'item.visLayer.pluginResource.urlPath', ''); |
AMoo-Miki
reviewed
Jul 7, 2023
| const onButtonClick = () => setIsErrorPopoverOpen((isOpen) => !isOpen); | ||
| const closeErrorPopover = () => setIsErrorPopoverOpen(false); | ||
|
|
||
| const errorMsg = get(props, 'visLayer.error.message', undefined) as string | undefined; |
AMoo-Miki
reviewed
Jul 7, 2023
| } | ||
|
|
||
| export function EventsPanel(props: Props) { | ||
| return ( |
There was a problem hiding this comment.
Do we not need to test this?
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+1
to
+6
| $vis-description-width: 200px; | ||
| $event-vis-height: 55px; | ||
| $timeline-panel-height: 100px; | ||
| $content-padding-top: 110px; // Padding needed within view events flyout content to sit comfortably below flyout header | ||
| $date-range-height: 45px; // Static height we want for the date range picker component | ||
| $error-icon-padding-right: -8px; // This is so the error icon is aligned consistent with the event count icons |
There was a problem hiding this comment.
Do the flyout and date-picker change sizes with text being enlarged? If yes, we should use em or rem here.
AMoo-Miki
reviewed
Jul 7, 2023
| } | ||
|
|
||
| embeddable.updateInput({ | ||
| // @ts-ignore |
There was a problem hiding this comment.
We should document the reason for having the ts-ignore
AMoo-Miki
reviewed
Jul 7, 2023
| const visLayers = (get(embeddable, 'visLayers', []) as VisLayer[]).filter((visLayer) => | ||
| isPointInTimeEventsVisLayer(visLayer) | ||
| ) as PointInTimeEventsVisLayer[]; | ||
| if (visLayers !== undefined) { |
There was a problem hiding this comment.
Wouldn't visLayers always be an array making this condition always true?
AMoo-Miki
reviewed
Jul 7, 2023
| const map = new Map<string, EventVisEmbeddableItem[]>() as EventVisEmbeddablesMap; | ||
| // Currently only support PointInTimeEventVisLayers. Different layer types | ||
| // may require different logic in here | ||
| const visLayers = (get(embeddable, 'visLayers', []) as VisLayer[]).filter((visLayer) => |
AMoo-Miki
reviewed
Jul 7, 2023
Comment on lines
+144
to
+146
| filters: embeddable.getInput().filters, | ||
| query: embeddable.getInput().query, | ||
| timeRange: embeddable.getInput().timeRange, |
There was a problem hiding this comment.
Is embeddable.getInput() expensive? if so, we should fetch it once into a variable and reuse it.
AMoo-Miki
reviewed
Jul 7, 2023
| const embeddableVisFactory = getEmbeddable().getEmbeddableFactory('visualization'); | ||
| try { | ||
| const { left, right } = getValueAxisPositions(embeddable); | ||
| const map = new Map<string, EventVisEmbeddableItem[]>() as EventVisEmbeddablesMap; |
There was a problem hiding this comment.
map is not a good choice for a variable name; let's find a better name for it.
AMoo-Miki
reviewed
Jul 7, 2023
| }, | ||
| }); | ||
|
|
||
| const curList = (map.get(pluginResourceType) === undefined |
There was a problem hiding this comment.
Do we care for it to be specifically undefined? if not, .has() would be a better choice.
ashwin-pc
approved these changes
Jul 7, 2023
There was a problem hiding this comment.
@ohltyler please address @AMoo-Miki's comments in a fast follow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Manual backport of #4361 . There was a changelog conflict.
Check List
yarn test:jestyarn test:jest_integrationyarn test:ftr