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

[Bug] Services Data Picker, UI Fixes #2177

Merged
merged 13 commits into from
Oct 5, 2024

Conversation

TackAdam
Copy link
Collaborator

@TackAdam TackAdam commented Sep 20, 2024

Description

  1. Bugfix for services data picker where declaring it twice was causing it to not render after closing fly-out and reset the selected data source to default. Also fix data source changing when navigation to traces through fly-out.
  2. UI fix to Metrics switched refresh button to icon for super date picker
  3. UI fix to overview page changing title of dashboard and adding ui link pop out icon
  4. Add URL redirection paths to account for changed functionality. (Comment Below)
  5. Add testing for the url re-direction.(Comment Below)
    Bugfix:
Screen.Recording.2024-09-19.at.8.45.21.PM.mov

Metrics page after:
Screenshot 2024-09-19 at 8 58 43 PM

Before:
Screenshot 2024-09-19 at 9 06 50 PM

Overview page after:
Screenshot 2024-09-19 at 8 57 44 PM
Screenshot 2024-09-19 at 9 05 58 PM

Before:
Screenshot 2024-09-19 at 9 06 35 PM

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

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.

@TackAdam TackAdam added bug Something isn't working backport 2.x labels Sep 20, 2024
Adam Tackett added 2 commits September 19, 2024 21:05
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
@TackAdam TackAdam marked this pull request as ready for review September 20, 2024 04:17
Adam Tackett and others added 3 commits September 19, 2024 21:22
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
Copy link
Collaborator

@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 @TackAdam , thanks for putting this together. Just left some minor comments

public/components/trace_analytics/home.tsx Outdated Show resolved Hide resolved
public/components/trace_analytics/home.tsx Show resolved Hide resolved
Copy link
Collaborator

@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.

When data source is disabled.
#data_source.enabled: false

Hi @TackAdam, If im understanding this correct, we are still relying on dataSource management's registration for checking the MDS feature flag status. Lets change the check from datasource management check into dataSource plugin, since the MDS status is not coming from data source management plugin's registration anymore.

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.24%. Comparing base (61328a9) to head (5d865cf).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...ponents/overview/components/dashboard_controls.tsx 0.00% 2 Missing ⚠️
public/components/metrics/top_menu/top_menu.tsx 0.00% 1 Missing ⚠️
...ace_analytics/components/services/service_view.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2177      +/-   ##
==========================================
- Coverage   56.58%   56.24%   -0.35%     
==========================================
  Files         389      390       +1     
  Lines       14954    15083     +129     
  Branches     4059     4111      +52     
==========================================
+ Hits         8462     8483      +21     
- Misses       6430     6538     +108     
  Partials       62       62              
Flag Coverage Δ
dashboards-observability 56.24% <20.00%> (-0.35%) ⬇️

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
Collaborator

@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.

Transferring the sync up with @TackAdam here:

const DataSourceMenuView = props.dataSourceManagement?.ui?.getDataSourceMenu<
The above is just for getting the picker. So I think we can just keep like this for now.

@TackAdam
Copy link
Collaborator Author

TackAdam commented Oct 3, 2024

Old Navigation No MDS

OldNavigationNoMDS.mov
Path Used After Clicking
http://localhost:5601/app/trace-analytics-dashboards http://localhost:5601/app/observability-traces#/traces
http://localhost:5601/app/observability-traces#/traces http://localhost:5601/app/observability-traces#/traces
http://localhost:5601/app/observability-traces#/services http://localhost:5601/app/observability-traces#/services
http://localhost:5601/app/observability-traces#/traces/03f9c770db5ee2f1caac0afc36db49ba http://localhost:5601/app/observability-traces#/traces?datasourceId=&traceId=03f9c770db5ee2f1caac0afc36db49ba
http://localhost:5601/app/observability-traces#/services/analytics-service http://localhost:5601/app/observability-traces#/services?datasourceId=&serviceId=analytics-service

Old Navigation MDS Enabled

OldNavigationMDSEnabled.mov
Path Used After Clicking
http://localhost:5601/app/trace-analytics-dashboards http://localhost:5601/app/observability-traces#/traces?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/traces http://localhost:5601/app/observability-traces#/traces?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/services http://localhost:5601/app/observability-traces#/services?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/traces/03f9c770db5ee2f1caac0afc36db49ba http://localhost:5601/app/observability-traces#/traces?datasourceId=&traceId=03f9c770db5ee2f1caac0afc36db49ba
http://localhost:5601/app/observability-traces#/services/analytics-service http://localhost:5601/app/observability-traces#/services?datasourceId=&serviceId=analytics-service

New Navigation No MDS

NewNavNoMDS.mov
Path Used After Clicking
http://localhost:5601/app/trace-analytics-dashboards http://localhost:5601/app/observability-traces-nav#/traces
http://localhost:5601/app/observability-traces#/traces http://localhost:5601/app/observability-traces-nav#/traces
http://localhost:5601/app/observability-traces#/services http://localhost:5601/app/observability-services-nav#/services
http://localhost:5601/app/observability-traces#/traces/03f9c770db5ee2f1caac0afc36db49ba http://localhost:5601/app/observability-traces-nav#/traces?datasourceId=&traceId=03f9c770db5ee2f1caac0afc36db49ba
http://localhost:5601/app/observability-traces#/services/analytics-service http://localhost:5601/app/observability-services-nav#/services?datasourceId=&serviceId=analytics-service

New Navigation MDS Enabled

NewNavMDSEnabled.mov
Path Used After Clicking
http://localhost:5601/app/trace-analytics-dashboards http://localhost:5601/app/observability-traces-nav#/traces?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/traces http://localhost:5601/app/observability-traces-nav#/traces?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/services http://localhost:5601/app/observability-services-nav#/services?datasourceId=f5895ea0-6706-11ef-87c9-97c467b0eaa1
http://localhost:5601/app/observability-traces#/traces/03f9c770db5ee2f1caac0afc36db49ba http://localhost:5601/app/observability-traces-nav#/traces?datasourceId=&traceId=03f9c770db5ee2f1caac0afc36db49ba
http://localhost:5601/app/observability-traces#/services/analytics-service http://localhost:5601/app/observability-services-nav#/services?datasourceId=&serviceId=analytics-service

public/plugin.tsx Outdated Show resolved Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
Comment on lines +64 to +70
export const observabilityTracesNewNavURL = observabilityTracesNewNavID;
export const observabilityTracesID = 'observability-traces';
export const observabilityTracesTitle = 'Traces';
export const observabilityTracesPluginOrder = 5093;

export const observabilityServicesNewNavID = 'observability-services-nav';
export const observabilityServicesNewNavURL = observabilityServicesNewNavID + '#/services';
export const observabilityServicesNewNavURL = observabilityServicesNewNavID;
Copy link
Member

Choose a reason for hiding this comment

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

If we are assigning the observabilityTracesNewNavURL = observabilityTracesNewNavID; Can we just use the observabilityTracesNewNavID directly?

Copy link
Member

Choose a reason for hiding this comment

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

nit. Not sure why we have ID capitalized should be Id for at all places.

Copy link
Collaborator Author

@TackAdam TackAdam Oct 4, 2024

Choose a reason for hiding this comment

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

Removed the NewNavURL variables.

Looks like all ID's are in this file Ill do a separate PR to fix as it will affect a lot of files and don't want the other changes to get overshadowed by it.

Comment on lines +28 to +46
if (hash.includes('#/services/')) {
const serviceId = location.hash.split('/services/')[1]?.split('?')[0] || '';
if (serviceId) {
if (isNewNavEnabled) {
window.location.assign(
`/app/${observabilityServicesNewNavID}#/services?datasourceId=&serviceId=${encodeURIComponent(
serviceId
)}`
);
} else {
window.location.assign(
`/app/${observabilityTracesID}#/services?datasourceId=&serviceId=${encodeURIComponent(
serviceId
)}`
);
}
return;
}
}
Copy link
Member

@ps48 ps48 Oct 3, 2024

Choose a reason for hiding this comment

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

URL query parameters are not ordered by default. Browsers, servers, and other systems generally treat query parameters as unordered key-value pairs.

https://example.com/page?param1=value1&param2=value2
https://example.com/page?param2=value2&param1=value1

Will this work with and without serviceID being the first param?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This check is for if a id is provided. The new navigation check is to handle the different urls used for both
observabilityServicesNewNavID vrs observabilityTracesID
This isn't affected by if MDS is enabled, as the legacy re-direction should only be for local host id's.

Signed-off-by: Adam Tackett <[email protected]>
@TackAdam TackAdam merged commit cf01e52 into opensearch-project:main Oct 5, 2024
11 of 18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 5, 2024
* bug fix services data picker, ui fixes

Signed-off-by: Adam Tackett <[email protected]>

* add tooltip to the re-direct

Signed-off-by: Adam Tackett <[email protected]>

* fix services view page revert

Signed-off-by: Adam Tackett <[email protected]>

* fix services view page revert

Signed-off-by: Adam Tackett <[email protected]>

* fix services view

Signed-off-by: Adam Tackett <[email protected]>

* applied comments

Signed-off-by: Adam Tackett <[email protected]>

* fix traces bug

Signed-off-by: Adam Tackett <[email protected]>

* Fix bugs and add url redirection and testing

Signed-off-by: Adam Tackett <[email protected]>

* remove comment

Signed-off-by: Adam Tackett <[email protected]>

* address comments

Signed-off-by: Adam Tackett <[email protected]>

---------

Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Shenoy Pratik <[email protected]>
Co-authored-by: Adam Tackett <[email protected]>
Co-authored-by: Shenoy Pratik <[email protected]>
(cherry picked from commit cf01e52)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
TackAdam pushed a commit that referenced this pull request Oct 7, 2024
* [Bug] Services Data Picker, UI Fixes (#2177)

* bug fix services data picker, ui fixes

Signed-off-by: Adam Tackett <[email protected]>

* add tooltip to the re-direct

Signed-off-by: Adam Tackett <[email protected]>

* fix services view page revert

Signed-off-by: Adam Tackett <[email protected]>

* fix services view page revert

Signed-off-by: Adam Tackett <[email protected]>

* fix services view

Signed-off-by: Adam Tackett <[email protected]>

* applied comments

Signed-off-by: Adam Tackett <[email protected]>

* fix traces bug

Signed-off-by: Adam Tackett <[email protected]>

* Fix bugs and add url redirection and testing

Signed-off-by: Adam Tackett <[email protected]>

* remove comment

Signed-off-by: Adam Tackett <[email protected]>

* address comments

Signed-off-by: Adam Tackett <[email protected]>

---------

Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Shenoy Pratik <[email protected]>
Co-authored-by: Adam Tackett <[email protected]>
Co-authored-by: Shenoy Pratik <[email protected]>
(cherry picked from commit cf01e52)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Update integ snapshot

Signed-off-by: Adam Tackett <[email protected]>

---------

Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Shenoy Pratik <[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: Adam Tackett <[email protected]>
Co-authored-by: Shenoy Pratik <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants