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

Update Observability Data Sources with Type and Redirection #8492

Merged

Conversation

paulstn
Copy link
Contributor

@paulstn paulstn commented Oct 4, 2024

Description

Added data source type to the data sources page

For S3:
image
And Prometheus:
image

Updating redirection to Discover now that Explorer cannot be redirected to

For the flows in associated objects and accelerations that require redirection, all instances can be separated out into two kinds:

  • for tables and skipping indices: redirection to a 'flint' datasource with the datasource, database, and table name.
  • for covering indices and materialized views: redirection to the associated index name.
    For both kinds of redirection, now the MDS id has to be used to query correctly. All of these flows were updated accordingly.

There is one last kind of redirection with this card:
image
That previously would redirect to Log Explorer with only the datasource and nothing else. In the current version of Discover, doing this would be impossible, and even alternatives such as opening the DataSet picker modal in discover with the datasource selected isn't feasible. Currently it is set to redirect to Discover without the datasource.

Issues Resolved

Resolves: #8256

Screenshot

Testing the changes

Testing with MDS off:

Screen.Recording.2024-10-15.at.8.06.04.PM.mov

(video cut for length but query eventually completes)

When query:enhancements:enabled is off, every redirection point is disabled as such:
image
This is done for

  • assc. obj. table
  • assc. obj. flyout
  • acc. table
  • acc. flyout

Changelog

  • fix: Updated DataSource Management to redirect to Discover as well as display the type of the DataSource

Check List

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

Copy link
Contributor

github-actions bot commented Oct 4, 2024

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 77.41935% with 7 lines in your changes missing coverage. Please review.

Project coverage is 60.87%. Comparing base (650166d) to head (d2197e8).
Report is 22 commits behind head on main.

Files with missing lines Patch % Lines
..._management/utils/associated_objects_tab_utils.tsx 70.00% 1 Missing and 2 partials ⚠️
...ted_object_management/associated_objects_table.tsx 60.00% 2 Missing ⚠️
...nts/acceleration_management/acceleration_table.tsx 75.00% 1 Missing ⚠️
...nnection_detail/direct_query_connection_detail.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8492   +/-   ##
=======================================
  Coverage   60.87%   60.87%           
=======================================
  Files        3790     3790           
  Lines       90215    90228   +13     
  Branches    14142    14146    +4     
=======================================
+ Hits        54915    54924    +9     
- Misses      31836    31840    +4     
  Partials     3464     3464           
Flag Coverage Δ
Linux_1 29.15% <14.28%> (?)
Linux_2 56.39% <ø> (ø)
Linux_3 37.72% <77.41%> (?)
Linux_4 29.90% <14.28%> (-0.01%) ⬇️
Windows_1 29.17% <14.28%> (-0.01%) ⬇️
Windows_2 56.34% <ø> (ø)
Windows_3 37.72% <77.41%> (+<0.01%) ⬆️
Windows_4 29.90% <14.28%> (-0.01%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link
Contributor

❌ Entry Too Long

Entry is 129 characters long, which is 29 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length.

Copy link
Contributor

❌ Entry Too Long

Entry is 119 characters long, which is 19 characters longer than the maximum allowed length of 100 characters. Please revise your entry to be within the maximum length.

Signed-off-by: Paul Sebastian <[email protected]>
Signed-off-by: Paul Sebastian <[email protected]>
Copy link
Contributor

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @paulstn , thanks for taking this on. I just left some comments. In addition, could you also add

In the PR description, so that the above issue can be auto resolved by merging this PR.

@RyanL1997
Copy link
Contributor

RyanL1997 commented Oct 16, 2024

Also, can we record some testing videos, for both MDS enabled and disabled?

Copy link
Member

@sejli sejli left a comment

Choose a reason for hiding this comment

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

Looks good so far, just some general questions and nitpicks.

@@ -231,8 +232,9 @@ export const AccelerationDetailsFlyout = (props: AccelerationDetailsFlyoutProps)
const DiscoverIcon = () => {
return (
<EuiButtonEmpty
isDisabled={!getUiSettings().get('query:enhancements:enabled')}
Copy link
Member

Choose a reason for hiding this comment

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

nit: data_source_management doesn't take a dependency on the data plugin, but could maybe add it here since we also have DEFAULT_DATA_SOURCE_UI_SETTINGS_ID.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would there be any benefit to a dependency on the data plugin if we can just check the ui settings? Or were you pointing me to adding this UI setting as a constant to that folder?

Copy link
Member

Choose a reason for hiding this comment

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

Was just pointing you to add it as a constant and export it in the data source management plugin.

@@ -0,0 +1,2 @@
fix:
- Updated DataSource Management to redirect to Discover as well as display the type of the DataSource ([#8492](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8492))
Copy link
Member

Choose a reason for hiding this comment

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

Just FYI, the release notes generator requires the CHANGELOG description to be under 100 characters. Not sure if it counts the PR number, but the description itself is at 99 characters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah my original description was over 100 characters and when I shortened it the changelog was accepted so I'd assume that the changebot checking the character count would be consistent with the release notes generator. That's just an assumption though.

datasourceType: DATA_SOURCE_TYPES.S3Glue,
},
application.navigateToApp('data-explorer', {
path: `discover#?_a=(discover:(columns:!(_source),isDirty:!f,sort:!()),metadata:(view:discover))&_g=(filters:!(),refreshInterval:(pause:!t,value:0),time:(from:now-15m,to:now))&_q=(filters:!(),query:(dataset:(dataSource:(id:'${
Copy link
Member

Choose a reason for hiding this comment

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

nit: Feel like these two functions have a lot of duplicate code, could be worth a try to either combine them or wrap the actual path in another function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally when it was just the 'state' being redirected to explorer through navigateToApp, that made a lot of sense. However, since Discover's url redirection has different forms between redirecting to a flint index vs an index pattern, the differences i would need to put in between both strings would be too much for it to be worth it, as I think it would greatly reduce readability.

Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Proxy approval for Ryan since he isnt a maintainer yet but is more familiar with this section of the codebase than me.

@ashwin-pc ashwin-pc merged commit 5d74306 into opensearch-project:main Oct 21, 2024
71 of 72 checks passed
@paulstn paulstn deleted the update-obs-datasources-redir-page branch October 21, 2024 23:19
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 21, 2024
* add type to datasources description area

Signed-off-by: Paul Sebastian <[email protected]>

* complete merge

Signed-off-by: Paul Sebastian <[email protected]>

* keep imports

Signed-off-by: Paul Sebastian <[email protected]>

* update redirection to discover

Signed-off-by: Paul Sebastian <[email protected]>

* let only datasource redirection to explorer use plain discover

Signed-off-by: Paul Sebastian <[email protected]>

* Changeset file for PR #8492 created/updated

* update for case where flint is within default cluster

Signed-off-by: Paul Sebastian <[email protected]>

* rename helper functions

Signed-off-by: Paul Sebastian <[email protected]>

* fix tests

Signed-off-by: Paul Sebastian <[email protected]>

* disabled redir on query enhancements disabled

Signed-off-by: Paul Sebastian <[email protected]>

* shelter against rison encoding error

Signed-off-by: Paul Sebastian <[email protected]>

* try catch getting ui settings

Signed-off-by: Paul Sebastian <[email protected]>

* update tests

Signed-off-by: Paul Sebastian <[email protected]>

---------

Signed-off-by: Paul Sebastian <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 5d74306)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ananzh pushed a commit that referenced this pull request Oct 24, 2024
…8673)

* add type to datasources description area



* complete merge



* keep imports



* update redirection to discover



* let only datasource redirection to explorer use plain discover



* Changeset file for PR #8492 created/updated

* update for case where flint is within default cluster



* rename helper functions



* fix tests



* disabled redir on query enhancements disabled



* shelter against rison encoding error



* try catch getting ui settings



* update tests



---------



(cherry picked from commit 5d74306)

Signed-off-by: Paul Sebastian <[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>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Redirection issue for direct query datasource in dashboards management
4 participants