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

[MD]Allow create and distinguish index pattern with same name but from different datasources #3604

Merged

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Mar 13, 2023

Description

Prepend datasource name to index pattern title in UI to distinguish across different data source. This is aligned with UX designer

  • Refactor validation on creating index pattern, to allow duplicate index pattern title with different datasource.
  • Add getIndexPatternTitle() util function in data plugin -> index pattern service to prepend datasource name to index pattern title for display purpose. E.g my-datasource.my-index-pattern
  • refactor components that display index pattern title (Discover, saved object manager, save object finder) by using getIndexPatternTitle() util function provided by data plugin

Screenshots

  • current "create index pattern" flow:
    image

  • updated to allow creating same index pattern name with different datasources
    image

  • updated saved object management view:
    image

  • updated index pattern display in Discover
    image

  • updated index pattern display in create visualization
    image

Issues Resolved

#2592
#2599

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@zhongnansu zhongnansu changed the title Prepend datasource name to index pattern title to distinguish across … Prepend datasource name to index pattern title in UI to distinguish across data sources Mar 13, 2023
@zhongnansu zhongnansu added multiple datasource multiple datasource project v2.7.0 labels Mar 13, 2023
@zhongnansu zhongnansu assigned zhongnansu and unassigned zhongnansu Mar 13, 2023
@zhongnansu zhongnansu force-pushed the md-dup-index-pattern-poc branch from 45323e7 to 1601f23 Compare March 14, 2023 00:10
@zhongnansu zhongnansu force-pushed the md-dup-index-pattern-poc branch from 1601f23 to c402636 Compare March 14, 2023 00:34
@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Merging #3604 (0d2faa9) into main (2db2c43) will decrease coverage by 0.02%.
The diff coverage is 49.05%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3604      +/-   ##
==========================================
- Coverage   66.43%   66.42%   -0.02%     
==========================================
  Files        3210     3210              
  Lines       61677    61722      +45     
  Branches     9522     9530       +8     
==========================================
+ Hits        40977    40996      +19     
- Misses      18419    18440      +21     
- Partials     2281     2286       +5     
Flag Coverage Δ
Linux 66.36% <49.05%> (-0.02%) ⬇️
Windows 66.37% <49.05%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../data_source_management/public/components/utils.ts 100.00% <ø> (ø)
...ins/saved_objects_management/server/routes/find.ts 6.89% <0.00%> (-2.63%) ⬇️
...on/index_patterns/index_patterns/index_patterns.ts 56.33% <33.33%> (-0.74%) ⬇️
...mponents/step_index_pattern/step_index_pattern.tsx 85.84% <33.33%> (ø)
...aved_objects/public/finder/saved_object_finder.tsx 72.97% <40.00%> (-3.50%) ⬇️
...tion/components/sidebar/discover_index_pattern.tsx 87.50% <66.66%> (-5.36%) ⬇️
src/plugins/data/common/index_patterns/utils.ts 75.00% <83.33%> (+75.00%) ⬆️
...mon/index_patterns/index_patterns/index_pattern.ts 58.69% <100.00%> (+0.45%) ⬆️

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@zhongnansu zhongnansu marked this pull request as ready for review March 14, 2023 02:31
@zhongnansu zhongnansu requested a review from a team as a code owner March 14, 2023 02:31
@zhongnansu zhongnansu requested review from ananzh and kavilla March 14, 2023 16:51
@kristenTian
Copy link
Contributor

Could we add some screenshots for the view across updated pages?

Signed-off-by: Su <[email protected]>
@zhongnansu zhongnansu force-pushed the md-dup-index-pattern-poc branch from 847ac7d to f29f3ac Compare March 16, 2023 02:57
@kristenTian
Copy link
Contributor

kristenTian commented Mar 16, 2023

In the case where plugins or core interact directly through indices but not through index pattern. Maybe we could open an issue to track it?

This could be solved by a global datasource picker in long term but still pending on the design. Looking for your thoughts on this :D - Overall LGTM

@zhongnansu
Copy link
Member Author

In the case where plugins or core interact directly through indices but not through index pattern. Maybe we could open an issue to track it?

This could be solved by a global datasource picker in long term but still pending on the design. Looking for your thoughts on this :D - Overall LGTM

This is an issue opened for a global datasource picker, still under discussion. #2841.

For plugins to interact with indices in datasource. We provide the sever side API getDataSourceClient() for them to get a client and send requets. Plugins can build their solution based on it, plugins need to implement by themselves to onboard such use case. We want to support visualization, visualizations are based on index-pattern, that's why we make changes to index-pattern. @kristenTian

@zhongnansu zhongnansu requested a review from kristenTian March 17, 2023 21:19
@zhongnansu zhongnansu requested a review from abbyhu2000 March 21, 2023 19:59
kristenTian
kristenTian previously approved these changes Mar 22, 2023
kristenTian
kristenTian previously approved these changes Mar 24, 2023
@zhongnansu zhongnansu removed the request for review from ananzh March 24, 2023 23:22
@zhongnansu zhongnansu assigned ananzh and unassigned zhongnansu Mar 31, 2023
@ananzh
Copy link
Member

ananzh commented Apr 1, 2023

Last comment, I think debouncedFetch is a bit hard test in unit tests. Maybe use Jest's useFakeTimers to control the timer used for debouncing. It is easier to test it in functional test. Either is fine to me. Not a blocker.

@zhongnansu zhongnansu changed the title Prepend datasource name to index pattern title in UI to distinguish across data sources Allow create and distinguish index pattern with same name but from different datasources Apr 3, 2023
@zhongnansu zhongnansu requested review from kristenTian and ananzh April 3, 2023 19:42
@zhongnansu zhongnansu changed the title Allow create and distinguish index pattern with same name but from different datasources [MD]Allow create and distinguish index pattern with same name but from different datasources Apr 3, 2023
Comment on lines +99 to +113
const savedObjects = await Promise.all(
findResponse.saved_objects.map(async (obj) => {
if (obj.type === 'index-pattern') {
const result = { ...obj };
result.attributes.title = await getIndexPatternTitle(
obj.attributes.title,
obj.references,
getDataSource
);
return result;
} else {
return obj;
}
})
);
Copy link
Member

Choose a reason for hiding this comment

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

Similar to my other comment above, this logic shouldn't be here, it should be accounted for in the saved object management definition: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/server/saved_objects/index_patterns.ts#L42-L44

Comment on lines +167 to +174
if (obj.type === 'index-pattern') {
const result = { ...obj };
result.attributes.title = await getIndexPatternTitle(
obj.attributes.title!,
obj.references,
getDataSource
);
return result;
Copy link
Member

Choose a reason for hiding this comment

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

Doing an explicit type check within the generalized saved_object plugin seems like an antipattern. Ideally, any logic unique to a particular saved object should exist in that saved object definition (index-pattern, in this case).

Luckily, there are better patterns available to do what we're doing here. One option is to provide a custom title getter method in the index-pattern saved object definition, if we always want the title to have the appended data source. Alternatively, if we sometimes need just the index pattern title, and other times need the appended version, we can just define a new custom property, such as "displayTitle" and have this logic in the getter for that.

As an example, see how the visualize plugin provides its own custom finder method as part of the saved object loader: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/visualizations/public/saved_visualizations/saved_visualizations.ts

Copy link
Member Author

@zhongnansu zhongnansu Apr 5, 2023

Choose a reason for hiding this comment

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

Prepending datasource to title requires fetching datasource name first, which replies on a saved object client to make a call. This should happen where there's a client to use.

  • saved object management definition doesn't provide client , passing it to magement -> getTitlte() will change the interface that shard by other saved object. And to pass it for index-pattern type only, again need explicit check.
  • index-pattern saved object can't provide client, because unfortunately unlike savedVis that you referred to, there's no SavedIndexPattern extends SavedObjectClass exists. Don't think it's worth to create one only for prepending datasource to the title for display purpose. Comparing to visualization, index patterns are comparatively simpler objects

Besides, there's an issue under discussion about having an OUI component that can be used to select datasource then index pattern when creating Visualzaiton/Dashboards/Discover. #2841. The logic we have here may need be refactored after that discussion is settled.

Thanks for bringing this up, this might not be a good pattern to use, but it might not be a blocker for this PR. Can we create an issue to track and address in the future improvements? Does it sound good to you? @joshuarrrr

@zhongnansu
Copy link
Member Author

@joshuarrrr @ananzh @kristenTian Add some more context to help with review

Two issues we are addressing in this PR

  1. A pain-point is user today couldn't even create an index pattern with same name but from let's say 2 different datasource.
  2. After 1) is resolved, user will get 2+ index patterns with same name. They need a way to distinguish them based on its datasource. The idea is, the actual title saved for each index pattern(no matter which datasource it's created from) should not be changed to avoid large blast radius(wherever title is used as a search field). So we only change the index pattern title that being displayed, based on the datasource it's created from.

@zhongnansu zhongnansu merged commit b656658 into opensearch-project:main Apr 6, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 6, 2023
…m different datasources (#3604)

*[Multiple Datasource] Allow create and distinguish index pattern with same name but from different datasources

Signed-off-by: Su <[email protected]>
(cherry picked from commit b656658)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
zhongnansu pushed a commit that referenced this pull request Apr 6, 2023
…m different datasources (#3604) (#3801)

*[Multiple Datasource] Allow create and distinguish index pattern with same name but from different datasources


(cherry picked from commit b656658)

Signed-off-by: Su <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
…m different datasources (opensearch-project#3604)

*[Multiple Datasource] Allow create and distinguish index pattern with same name but from different datasources

Signed-off-by: Su <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MD] Allow create index patterns for indices with same name but from different datasources.
6 participants