-
Notifications
You must be signed in to change notification settings - Fork 58
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
[FEATURE] MDS support in Integrations for observability plugin #2051
Conversation
029c0ca
to
95487ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, have a few questions
componentConfig={{ | ||
activeOption: [ | ||
{ | ||
id: data?.references?.[0]?.dataSourceMDSId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How stable is the assumption that the first reference is always the correct dataSource? I could see more reference types being added in the future -- if we keep this is it at least under test so we'll know if we need to update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it has been updated in the test as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this line coming from the data source picker? From the call back function for getting the selected cluster id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is coming in from the saved objects created during the set up process of the integrations.
@@ -95,7 +98,9 @@ const suggestDataSources = async ( | |||
}) ?? [] | |||
); | |||
} else if (type === 's3' || type === 'securityLake') { | |||
const result = (await http.get(DATACONNECTIONS_BASE)) as Array<{ | |||
const result = (await http.get( | |||
`${DATACONNECTIONS_BASE}/dataSourceMDSId=${dataSourceMDSId ?? ''}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this usage correct? This is a URL with an =
in it instead of a request param? The http.get
method already has a way of using request params with automatic undefined-handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanted to keep it consistent to how other plugins were using and by adding the mds-id here the path is getting differentiated for the other locations where this api is being used
lockConnectionType?: boolean; | ||
}) { | ||
const [dataSourceMDSId, setDataSourceMDSId] = useState<string | undefined>(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between undefined
and empty string here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make this consistent to be an empty string in all locations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between undefined and empty string here?
''
represents the local cluster. I guess the undefined
is trying to handle the case that when the MDS is not enabled and the ID is undefined, so at that time we just need to handle the request towards local cluster.
I can make this consistent to be an empty string in all locations
Ideally, we should still handle the undefined
and ''
separately. But just wanna make sure we check the MDS flag correctly so that it will not go thru the MDS endpoint when MDS is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tentative approval based on above
public/components/integrations/components/__tests__/testing_constants.ts
Outdated
Show resolved
Hide resolved
public/components/integrations/components/added_integration_overview_page.tsx
Outdated
Show resolved
Hide resolved
@@ -204,7 +206,8 @@ export const doExistingDataSourceValidation = async ( | |||
|
|||
const createComponentMapping = async ( | |||
componentName: string, | |||
payload: ComponentMappingPayload | |||
payload: ComponentMappingPayload, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create an issue in https://github.com/opensearch-project/opensearch-js/issues
to add missing components in the js client.
39545a4
to
935ff53
Compare
public/components/integrations/components/__tests__/testing_constants.ts
Outdated
Show resolved
Hide resolved
public/components/integrations/components/setup_integration.tsx
Outdated
Show resolved
Hide resolved
483bdf6
to
964bbe6
Compare
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
* Move the save button to the header control bar Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the save button and correct its size + position Signed-off-by: Ryan Liang <[email protected]> * Fix the date picker location Signed-off-by: Ryan Liang <[email protected]> * Rename the navigation in coreRef and switch to use compressed date picker Signed-off-by: Ryan Liang <[email protected]> * Fix the popover Signed-off-by: Ryan Liang <[email protected]> * Rename the button Signed-off-by: Ryan Liang <[email protected]> * Update to latest mockup Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the ui issues Signed-off-by: Ryan Liang <[email protected]> * Dummy metrics Signed-off-by: Ryan Liang <[email protected]> * Remove dummy Signed-off-by: Ryan Liang <[email protected]> * update snapshots Signed-off-by: Ryan Liang <[email protected]> * minor changes to match mocks Signed-off-by: Shenoy Pratik <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
9649ef8
to
b373625
Compare
Signed-off-by: Sumukh Swamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/dashboards-observability/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/dashboards-observability/backport-2.x
# Create a new branch
git switch --create backport/backport-2051-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 16e2bd556c0e2043e1e29764403ad4c6797e78b1
# Push it to GitHub
git push --set-upstream origin backport/backport-2051-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/dashboards-observability/backport-2.x Then, create a pull request where the |
…earch-project#2051) * Support for MDS in integrations Signed-off-by: sumukhswamy <[email protected]> * added changes for mds support in integrations Signed-off-by: sumukhswamy <[email protected]> * fixed case for Local cluster Signed-off-by: sumukhswamy <[email protected]> * addressed comments for undefined variables Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * changed the refrences object in integrations instance Signed-off-by: sumukhswamy <[email protected]> * addressed pr comments Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * [Page Header] New page header for metrics (opensearch-project#2050) * Move the save button to the header control bar Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the save button and correct its size + position Signed-off-by: Ryan Liang <[email protected]> * Fix the date picker location Signed-off-by: Ryan Liang <[email protected]> * Rename the navigation in coreRef and switch to use compressed date picker Signed-off-by: Ryan Liang <[email protected]> * Fix the popover Signed-off-by: Ryan Liang <[email protected]> * Rename the button Signed-off-by: Ryan Liang <[email protected]> * Update to latest mockup Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the ui issues Signed-off-by: Ryan Liang <[email protected]> * Dummy metrics Signed-off-by: Ryan Liang <[email protected]> * Remove dummy Signed-off-by: Ryan Liang <[email protected]> * update snapshots Signed-off-by: Ryan Liang <[email protected]> * minor changes to match mocks Signed-off-by: Shenoy Pratik <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> Signed-off-by: sumukhswamy <[email protected]> * Updated event analytics and integrations components Signed-off-by: sumukhswamy <[email protected]> * updated tests and conditions Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated the remote and local for snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots again Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated missing import Signed-off-by: sumukhswamy <[email protected]> --------- Signed-off-by: sumukhswamy <[email protected]> Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Signed-off-by: Sumukh Swamy <[email protected]> Co-authored-by: Jialiang Liang <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]>
…2051) (#2096) * [FEATURE] MDS support in Integrations for observability plugin (#2051) * Support for MDS in integrations Signed-off-by: sumukhswamy <[email protected]> * added changes for mds support in integrations Signed-off-by: sumukhswamy <[email protected]> * fixed case for Local cluster Signed-off-by: sumukhswamy <[email protected]> * addressed comments for undefined variables Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * changed the refrences object in integrations instance Signed-off-by: sumukhswamy <[email protected]> * addressed pr comments Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * [Page Header] New page header for metrics (#2050) * Move the save button to the header control bar Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the save button and correct its size + position Signed-off-by: Ryan Liang <[email protected]> * Fix the date picker location Signed-off-by: Ryan Liang <[email protected]> * Rename the navigation in coreRef and switch to use compressed date picker Signed-off-by: Ryan Liang <[email protected]> * Fix the popover Signed-off-by: Ryan Liang <[email protected]> * Rename the button Signed-off-by: Ryan Liang <[email protected]> * Update to latest mockup Signed-off-by: Ryan Liang <[email protected]> * Update snapshots Signed-off-by: Ryan Liang <[email protected]> * Fix the ui issues Signed-off-by: Ryan Liang <[email protected]> * Dummy metrics Signed-off-by: Ryan Liang <[email protected]> * Remove dummy Signed-off-by: Ryan Liang <[email protected]> * update snapshots Signed-off-by: Ryan Liang <[email protected]> * minor changes to match mocks Signed-off-by: Shenoy Pratik <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> Signed-off-by: sumukhswamy <[email protected]> * Updated event analytics and integrations components Signed-off-by: sumukhswamy <[email protected]> * updated tests and conditions Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated the remote and local for snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots again Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated snapshots Signed-off-by: sumukhswamy <[email protected]> * updated missing import Signed-off-by: sumukhswamy <[email protected]> --------- Signed-off-by: sumukhswamy <[email protected]> Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Signed-off-by: Sumukh Swamy <[email protected]> Co-authored-by: Jialiang Liang <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]> * linter fix Signed-off-by: sumukhswamy <[email protected]> --------- Signed-off-by: sumukhswamy <[email protected]> Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Shenoy Pratik <[email protected]> Signed-off-by: Sumukh Swamy <[email protected]> Co-authored-by: Jialiang Liang <[email protected]> Co-authored-by: Shenoy Pratik <[email protected]>
Description
Multi data source support in Integrations for observability plugin
Screen.Recording.2024-08-09.at.11.29.25.AM.mov
Issues Resolved
#1440
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.