Open ashwin-pc opened 1 year ago
Hey @ashwin-pc, thanks for the proposal!
Couple of things
Somewhat related; I know there is some security concerns with AngularJS but do we have specific user requirements we're working backwards from? Or some guiding beliefs that are informing this full redesign? We might want to include those to make it easier to provide feedback. For example, I think the unification of Event Explorer and Discover is one of those reasons, but I'm not sure this proposal address the overlaps and gaps between the two and how this will solve that problem. Right now, the background only focuses on the security concerns and doesn't explain why we'd make all the other changes as well.
Will the core concept of the data explorer build around index patterns still?
Should we include functional tests in the requirements? such as adding new functional tests, modifying the old discover functional tests for discover feature in data explorer, and also removing the irrelevant functional tests etc
Some quick and relatively minor feedback:
ui
property of the view data model hints in this direction, but we should clarify how much flexibility views have over their overall layout. Marking up the wireframe with the panel sections etc. would be helpful.extension
? The additional terminology is a bit confusing.The only thing that the application needs to do is to migrate its existing state management to the Data Explorer state management
This could use some more details/examples.
Feedback from the initial review:
It seems there are two very big pieces of work here (i.e., remediate AngularJS and upgrade Discover), but it seems we relegate the deangularize to another issue. Does the deangularize from AngularJS have any user impact to the proposed experience? If so, perhaps we can consider those separate issues and prompt for feedback accordingly.
Not really, upgrading discover take care of deangular tasks as well since we wont carry over any angular components. Part of disciver is already mgrated away from AngularJS so we can use those as is, and we need to only focus on the components that arent.
I think the "view registry" that allows apps to register themselves as views within Data Explorer is a good idea, but is this extensible to apps that want to modify data_explorer or change Nav options & Search/Query bar? I'm curious on the guardrails (if any) we are hoping to implement.
There are intentionally few guardrails here for now since it makes migrating existing applications onto Data Explorer easier. We can revisit the guardrails in future once we have more data about how this app is being used. There is enough isolation between views so changes in one should not affect the other, but the views we have today are sufficiently different that abstracting away more features into Data Explorer makes the integration more difficult. Also since Data explorer has so few abstractions, i dont think that any view will need to modify it to make their feature work, and that was the intention.
Is the toggle domain/cluster wide? I assume not but we may want to call it out. Additionally, is there some clear user value in having some a prominent option, even within an experimental state, if this will be version incompatible at some point? Given the impacts to routing and complexities of compatibility and user adoption, adding some friction might be okay for the long-term user experience. Even a link to a setting might be preferred over a toggle
Its at the tenant level, and will be present in advanced settings. We plan on removing the toggle once the deangularization work is complete and i will update the proposal to call that out since its pretty lite on that right now.
Are there specific questions you want the community to weigh in on? Without more details on current versus future it's hard to ascertain the impact. For instance, do we have a list of feature(s) that we are adding that we should be weighing in on as a community? e.g., review the proposals for changes to the Sidebar, Document table, and Discover Router CX?
Not really, there is only one question, and that is about how we want to handle the other views that discover supports (surrounding documents and single doc views). Right now i'm just looking for general feedback on the approach and if there are any things that i missed in my proposal.
Somewhat related; I know there is some security concerns with AngularJS but do we have specific user requirements we're working backwards from? Or some guiding beliefs that are informing this full redesign? We might want to include those to make it easier to provide feedback. For example, I think the unification of Event Explorer and Discover is one of those reasons, but I'm not sure this proposal address the overlaps and gaps between the two and how this will solve that problem. Right now, the background only focuses on the security concerns and doesn't explain why we'd make all the other changes as well.
Thats a good point, yes combining the event explorer view with discover is one such reason, but I didnt mention that explicitly here since Event Explorer is not a part of the base OSD repo and for developers who are using the minimal distribution, may never encounter that view. Thats why i focused on VisBuilder which is another such view that we want to be able to toggle between to explore the data. I also kept the description of view intentionally ambiguous since i want to make it possible for any view (e.g. Event Explorer) to easily integrate with Data Explorer regardless of their underlying architecture. That being said i will expand the overview section to highlight the current gaps with our different views
@abbyhu2000
Should we include functional tests in the requirements? such as adding new functional tests, modifying the old discover functional tests for discover feature in data explorer, and also removing the irrelevant functional tests etc
Tests should be a part of any feature that is built so i didnt want to call that out as an explicit requirement here
@joshuarrrr
Why is the property for embeddables called extension? The additional terminology is a bit confusing.
Its not a new term and is infact borrowed from Visualize. This is the property Visualize uses to list all the compatible visualization saved objects in the visualize listing view.
State management will need more tech details later The only thing that the application needs to do is to migrate its existing state management to the Data Explorer state management This could use some more details/examples. Doc table visualization - I think this will need it's own small technical design/proposal, as I have some thoughts about how we can do this to improve the experience and prevent future churn.
Yeah, detailed designs for some of the components will follow, this proposal just focusses on the high level design of the project
i will expand the overview section to highlight the current gaps with our different views
Another path would be to share some of the user-centric reasoning behind this large undertaking. Modernizing is important but trying to understand why this work specifically.
@ashwin-pc For some of the routing challenges of supporting old and new simultaneously, have you looked at https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/url_forwarding/README.md#L0-L1 ?
@ashwin-pc this looks very promising ... few point to mention :
Prometheus
, Cloudwatch
,Opensearch
...) and how do we align them to be displayed within one notion (see all the datasource that has an Observability
label or Security
label )Hey @YANG-DB can you help me understand what you mean by notion
here?
notion
is a semantic meaning that this visualization is related to. For example we can annotate a dashboard that shows visualization with the next notions: [http
,cpu
,Observability
,payment-services
]
similarly the data-source can have the next notions: [Observability
,Prometheus
, staging-env
]
So the this notion concept hide the metadata semantic relationship this visual element may represent and project
The labels technique is common practice for reflecting metadata notions
that are both dynamic for user definition and open for system-based default values...
@ashwin-pc Thanks for this writeup. A lot of work... just to document this UX repositioning and describe the impact.
Upon first read, it sounds like de-Angularization is being used as a primary motivation for the enhancement. I'm concerned that this is "burying the lead" on why we're moving in this new direction for UX on discovery. de-Angularization is important, yet it is technically a stand-alone effort and can be accomplished with no UX/UI impact. Seems like it's convenient timing, maybe opportunistic timing; we should be focusing the UX position on how and why it is better for the user, without concern over how it will be implemented, and why it is more-timely now than later.
@pjfitzgibbons you are right in stating that deangularization can be achieved without any UI/UX impact and was infact the recommended approach at first. However when it was discussed whether we should spend a whole lot of time first migrating our angular components to react, just to rewrite the whole thing again as a Data Explorer tool, it seems like throwaway effort which is why the two projects were combined. This then introduced a wrinkle that the deangularization had to happen by EOY since it was a security risk that we had committed to remove by then. This mean that to meet the deadline without refactoring very different applications (Log Explorer and Discover) into one, a data explorer skeleton that allowed two independent applications to run at the same time had to be developed. That is the design you see here.
[The org] then introduced a wrinkle that the deangularization had to happen by EOY since it was a security risk
This ^^ is the bit I was getting at. I feel declaring de-Ng as a "requirement" to the design effort of DataExplorer is misguided. I feel also, and more importantly, that it is valid and important to indicate the org-level commitment above, and how that agreement (de-Ng) has side-effect impacts on the ultimate design.
TLDR; It's ok to "And also this accomplishes an Org goal to remove security risk of Angular library". It's not ok to say "This, because Angular"
Will the core concept of the data explorer build around index patterns still?
Orthogonal to this, I would like to remove the dependency around index patterns. I think there's some work here, but I'd like to rethink index pattern as a concept. An index pattern really is a dynamic data schema, or an aggregation schema. It also gives us some advance features like run-time fields, scripted fields, etc. At the end of the day, we should invest in the ability to compose a query using other query languages, query indexes directly (and potentially be able to simply read the index mapping API to provide run-time schemas, which could be saved as a saved object for ease of exploration down the road). This will also give us more flexibility to correlate alerts, detectors, and other features from across other plugins.
Overview
Data Explorer project represents a strategic effort to address the security concerns associated with the legacy Discover component while delivering an enhanced and versatile data exploration experience for users. Data Explorer serves a dual purpose. Firstly, it aims to upgrade the existing Discover plugin to use React. Secondly, it seeks to become a comprehensive platform that caters to all data exploration needs.
All the existing features of Discover will seamlessly transition into Data Explorer. Additionally, this upgraded platform will introduce new capabilities, such as exploratory data analysis through aggregations and visualizations, as well as support for various query languages. The integration of these features within Data Explorer aims to provide a unified and efficient environment for data exploration tasks.
Background
The Discover component has been a longstanding and crucial part of OSD OpenSearch Dashboards. However, it was built using Angular version 1.0, which poses security concerns, requiring its removal by the end of the year Ref. Data exploration in OpenSearch has also been confusing with many different applications such as Discover, Visbuilder, Event Analytics all solving similar problems. Data explorer aims to solve both problems at once by offering a single platform where all data exploration views can exist simultaneously while also using it first as a target for the Discover deangularization effort.
Requirements
Architecture
For this feature the following changes will be made to the plugins:
data_explorer
: This is a new plugin introduced to act as the view container for the different sub applications for data exploration.discover
: The discover 2.0 plugin that is a combination of existing non angular code from discover_legacy, vis_builder and other parts of OSD that is useful and new components where existing components do not existvis_builder
: Migrates its views from a separate app to a view insidedata_explorer
discover_legacy
: The existingdiscover
plugin will move todiscover_legacy
and all associated routes will be renamed asdiscover_legacy
. While sharing, the URL will still referencediscover
.Division of responsibility
Data explorer simply acts as a shell for other exploration views, so here is how the responsibilities are divided between Data Explorer and its views.
Data Explorer: Data explorer is responsible for 4 primary features.
Views: Each of the views on the other had are responsible for:
The goal here is that by minimizing the surface area for Data Explorer specific changes, each application can migrate their existing view relatively easily onto data explorer.
Wireframe
View Registry
The Data Explorer exposes a view registry that allows apps to register themselves as views within Data Explorer. Each view object has the following properties:
Data model:
id
: The id of the view. This is used to identify the view in the view switcher and in the URLtitle
: The title of the view. This is used to display the view in the view switchericon
: The icon of the view. This is used to display the view in the view switcherui
: This is an object that contains the UI components for the view.panel
: This is the component that is rendered in the panel when the view is selectedworkspace
: This is the component that is rendered in the workspace when the view is selecteddefaults
: This is the default state for the view. This is used to initialize the state when the view is selectedreducer
: This is the reducer for the view. This is used to update the state of the view when actions are dispatcheddefaultPath
: This is the default path for the view. This is used to redirect the user to the view when they select it in the view switcherextention
: This is an object that contains the extension for the view. This is used to register the embeddable for the viewtype
: This is the type of the embeddabletoList
: This is a function that converts the saved object into a view list item. This is used to display each saved object when wants to load a supported saved objectshouldShow
: This is an optional function that is used to determine if the view should be shown in the view switcher. This is useful for views that are not compatible with the current data source. OpMigrating existing applications
Data explorer will likely be a place where existing application can migrate their data exploration views. This means that Data Explorer should also take into account how an application can do this without disrupting the users workflow. Such a migration is already needed for Discover and VisBuilder so I assume that we will need to do do this again for other views.
Migrating the actual application is pretty straight forward. Since Data Explorer is only responsible for 3 things, Data source, state management and view registry, the application can simply register itself as a view and then start using the Data Explorer state management and data source. The only thing that the application needs to do is to migrate its existing state management to the Data Explorer state management and reference the datasource passed down by Data Explorer instead of its own. The application needs to also modify its routes to use the Data Explorer routes and UI to match the panel and workspace components UI.
Routing
Routing is important for Data Explorer especially when we do not want to break backwards compatibility with existing applications. For this reason, Data Explorer will have its own routes that are prefixed with
/data_explorer
and the existing routes for each view is responsible for redirecting to the new routes. For example, the existing discover route will redirect to/data_explorer/discover
and the existing vis builder route will redirect to/data_explorer/vis_builder
by their respective plugins. This way, the existing routes can still be used and the new routes can be used for the new views.Discover 2.0 - Deangularize
With the above architecture in mind, here is how the existing discover plugin will be migrated to the new architecture.
Research issue: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4130
Mock wireframe
Toggle
Toggling between the old and new views is a bit more complicated than a standard migration since in this view we need to support both the old and new plugins at the same time. To do this we implement the routing strategy as mentioned before with one change, Data Explorer's router also checks to see if using the legacy plugin is turned on or not. If it is, it routes all discover traffic to
discover_legacy
, else it sends it off to the discover view registered to it. Once the migration is complete we simply remove this check.Migrating Features
Utilities that are either not tied to the UI or are already migrated like the JSON doc viewer will be migrated as is to the new experience.
Open questions