opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.69k stars 892 forks source link

[RFC] Charts & EUI: Forking vs Merging-in vs Folding-in #695

Closed AMoo-Miki closed 2 years ago

AMoo-Miki commented 3 years ago

Charts & EUI: Forking vs Merging-in vs Folding-in

Elastic UI Framework (EUI) is a collection of React UI components used throughout the interfaces of Elastic's web products and Elastic Charts is a set of chart components used within EUI and Elastic's web interfaces. Both EUI and Elastic Charts were built specifically for use in Kibana and primarily by employees of Elastic, and have historically been maintained in the open via Github. They have clear contribution guidelines, component development guides, component design docs, and style guides. Both EUI and Elastic Charts are the core UI and Visualization frameworks that comprise the bread-and-butter of the core OpenSearch Dashboards experience.

The UI framework used to be a separate package, named kibana-ui-framework, until it was merged into Kibana in 2016 for faster iterations, version locking, and unified styling. Late in 2017, a new React-based look-and-feel was created which resided in a repository separate from Kibana, named EUI, and some parts of Kibana started using it. The code for ui-framework eventually got moved to an internal package of its own within the Kibana source code and until just recently, Kibana contained components from both of these sources to support plugins that hadn't gotten updated to use EUI.

The purpose of this document is to review the recommendation for resolving the recently announced licensing constraints on Elastic Charts and EUI, both dependencies used in OpenSearch Dashboards. OpenSearch Dashboards uses older versions of these libraries and to be able to develop new features and fix bugs, the project would need to create a copy of the last open-sourced version of them; this can be done in a couple of ways which are discussed below.

Principles

  1. The solution should be easy to maintain
  2. Should have minimal disruptions to existing workflows
  3. Should not increase developer friction
  4. Should not limit future expandability

The Recommendation

None of the main options have any strong contraindications for adoption and none violate the principles unpleasantly.

Considering the pros and cons, the recommendation for the Charts and UI is Folding-in; this is a two-way door and reversible with minimal effort.

Options

Elastic Charts and Elastic UI Framework (EUI) are hosted in individual repositories and get pulled into OpenSearch Dashboards as NPM dependencies during installation. Currently in Kibana, updates to these packages get built and released individually to NPM and a version bump tracks the iterations; Kibana is then updated to reference the new version. Dashboards also has a built-in package manager, @osd/pm, which is inspired by Lerna but works with Yarn, link-ed dependencies, and workspaces to have fine-grained control. This package manager was built for several reasons, mainly to get away from wastefulness of webpack and towards individual in-production distributable bundles:

Long-term we want to get rid of Webpack from production (basically, it's causing a lot of problems, using a lot of memory and adding a lot of complexity). Ideally we want each plugin to build its own separate production bundles for both server and UI. To get there all Kibana plugins (including x-pack) need to be able to build their production bundles separately from Kibana, which means they need to be able to depend on code from Kibana without import-ing random files directly from the Kibana source code.

The Kibana project management tool

The options considered in this document are to either fork and publish which is the natural NPM/Yarn way, to pull in the code and merge it in with the existing code, or to use Dashboards' package manager which was built for this specific purpose.

Folding-in

This would involve pulling in the code for Charts and EUI, and merging them into the packages folder of OpenSearch Dashboards where they will be consumed by the built-in package manager. Kibana uses its built-in package manager for over 50 packages and OpenSearch Dashboards which inherited the built-in package manager from Kibana, uses it for over 20 packages. Hence, this will not be alien to Dashboards developers and contributors, and should not disrupt their workflows.

Plugin developers would link (NPM, Yarn) the project's UI components to their packages during development which involves a one-time execution of a command or two; if included unambiguously in the getting-started documentation, this would eliminate any real friction. Additionally, this will unify how developers use any of Dashboards' packages since all will be distributed as @osd/pm packages; Charts and EUI will no longer be among the exceptions that gets imported differently.

The code for Charts and EUI, residing in the packages folder, will be entirely self-contained, complete with their own tests and package.json which would make it as easy, if not more, to maintain when compared to having the code in their own repositories - since no extra building and publishing is needed. There is also precedent for folding packages into Kibana, datemath, which could help reduce the long term maintenance of Dashboards by reducing the effort cycles needed to publish it. Also, being self-contained, folding-in will not impose any limits on any future expanding.

On the other hand, even though the code will be open-sourced, by being nested inside Dashboards, Charts and EUI will be prohibitively expensive for the community to consume as a dependency and as such, the opportunity to have them independently contribute to open-source is lost. However, since an absence of the Charts and EUI libraries will not leave behind a gap, it is fair to assume they won't be missed. The effort for Dashboards to consume the code from its new location would be minimal, perhaps limited to a string find-and-replace.

Forking

Forking the packages under OpenSearch Project would allow the project to maintain the status quo: plugin developers and contributors can continue to use their existing workflows to develop and test their plugins and so will OpenSearch Dashboards; this causes no disruption. While this allows for plugin development to occur independent of the underlying Dashboards, these plugins cannot function fully outside of OpenSearch Dashboards and need to be loaded for complete execution and testing. Additionally, plugin developers who choose to distribute certain versions of these packages in their code will potentially face conflicts during installation or runtime, or force extra payloads on to their users - via loading duplicate libraries for similar functions and degrading performance.

Forking and publishing these packages would improve the project's standing with yet another contribution to the open-source community. However, adding features requested by the community that don't specifically serve the needs of OpenSearch Dashboards would cost the project in effort while not addressing them would stain its reputation.

That said, until recently, Elastic Charts and Elastic UI Framework (EUI) had a warning on their repositories against others using these packages for anything and as such, no non-related package consumes Charts and only a couple consume EUI. Considering that these components are specifically made to serve Kibana's at times unique needs, developers have better generic options for their UI components like Material-UI, React Bootstrap, and Ant Design.

To implement this option, the project would need to fork two repositories, change some names, reset the versions, push the changes, and publish to NPM. Afterwards, with every change that is pushed, a version bump and publishing to NPM will be required. However, consuming a published-to-NPM package is natural for JS developers - by simply including it as a dependency in their own package configuration; this adds no developer friction. Maintaining separate repositories, this method doesn't impose any limits on future expandibility.

Merging-in

To avoid having to maintain separate build and release pipelines, the project could simply pull in the code for Chart and EUI, and merge them into OpenSearch Dashboards. The code will mix with that of Dashboards and this entanglement will make it a little more complex for the project to maintain. With minimal exposure, using exports, plugin developers can link the project's UI components to their packages during development (NPM, Yarn). This involves a one-time execution of a command or two which if included in the getting-started instructions, would add very little disruption to the workflows developers employ; beyond that, it should be transparent to plugin developers.

This option differs from Folding-in in that the code, tests, and packaging logic for Charts are mixed in with those of Dashboards'; this could potentially complicate any future changes the project may want to make to how plugins share assets with Dashboards. Merging in just increases the number of source files in the Dashboards' code which is only ugly; if the code is thrown into a specific folder of its own, it wouldn't be any more difficult to maintain or expand than having it in a separate repository or package; in fact, one could argue that there would be nothing to maintain anymore since there would be no extra build and publish steps needed.

On the flip side, even though the code will be open-sourced, by being nested inside Dashboards, it takes away an opportunity for the project to contribute to the open-source community but the space already has strong contributors and a gap will not be felt. This would require a non-zero initial effort to implement; string replacement to the references would be cheap but testing, building, and production-packaging will need customization.

Others

There are a few other options that are simply unreasonable or complicated, and they are briefly mentioned here for the sake of completeness.

JacobBrandt commented 3 years ago

I agree that folding-in makes the most sense here for the EUI.

There is one other option that I think should be discussed before folding-in Charts though. The Charts library is not heavily used within this repository like EUI is. Maybe one day Charts will be used more with future enhancements like #379. However, that would be another topic of discussion for that roadmap on whether or not that effort would want to depend on Charts and fold-in the large library or go with another open-source approach. (ECharts, Vega, D3.js, etc.).

Right now OpenSearch Dashboards uses Charts for:

  1. TSVB
  2. Discover's column chart visualization (the original column_chart.js visualization still exists).

Since the use of this library within this repository is very limited and there is no immediate need to use it for something else what if this repository just depended on the last Apache 2.0 license version of Charts?

galangel commented 3 years ago

just my 2 cents. I'm ok with having it as yet another package, but it will be a huge one keep in mind. so I'll give it my vote =)

there are pros and cons to everything but, I still hold the opinion that we should fork it. and maintaining yet another repo isn't so bad.

In my vision, the OpenSearch components library should be separate, and this style guide should be one of the faces of the project, I would like devs that have no relation to OpenSearch to use this library, and they could offer unbiased perspectives and improvements. if osd has unique needs it's something else that we should address.

By limiting its usage only to OSD in favor of control we are limiting the possibility of growth. I can't predict the future but I hope this will evolve into its own thing.

JacobBrandt commented 3 years ago

@galangel Honestly, I could get behind a fork of EUI as well. It just seemed like that was a hard no from the team.

EUI can hold its ground against the other React based UI component libraries. The point that there is a warning about using EUI outside of Elastic doesn't mean much. FWIW, Elastic still talks about the Kibana plugin API as an experimental, use at your own risk, feature. I think the main concern from the team about forking was who would manage/own this fork. The OpenSearch team is right in that it takes extra work (not just development) for them to do it. However, it doesn't have to be an OpenSearch repository :wink: . When MapboxGL changed its license Elastic didn't create an Elastic fork. The open-source community created MapLibre and now Kibana depends on MapLibre.

spapadop commented 2 years ago

Hello, this RFC issue was discussed over a month ago at a community meeting, mentioning that discussions will conclude on following days in order to go ahead. Shall this move forward one way or another? The thing is that it's blocking any further relative developments until it's closed.

ahopp commented 2 years ago

Hey team! Looks like the recommendation (i.e. pulling in the code for Charts and EUI, and merging them into the packages folder of OpenSearch Dashboards) is the path for the time being so I'm closing this ticket. While this doesn't close the door on future libraries (e.g. new viz options) or enhancements, it should unblock any current, related development that is current in a holding pattern.

@AMoo-Miki please share an update on the next steps.

AMoo-Miki commented 2 years ago

Thanks @ahopp.

The upgrade to Node v14 being on track for Dashboards v2.0 enables us to pull the last Apache-2.0 releases of EUI and Charts by then.

atifsyedali commented 2 years ago

I am a bit late to add to this discussion but some thoughts...

First, doesn't folding in and merging also mean that all git history will be lost? The history helps a great deal when understanding certain changes during maintenance. Forking doesn't have this problem.

Second, there are quite a few community PRs (649 out 3027) made into EUI to date, which can easily be going into an EUI fork instead. There are tons of people using EUI for their projects, and finding and fixing bugs in EUI. Most people will not be upgrading from the last Apache-licensed EUI to Elastic-licensed EUI, which means there is a great opportunity to get community to simply find/replace their imports. And then these community PRs will now start going into EUI fork instead.

Thirdly, there are some design files in EUI that have had Elastic trademarks (icons, palettes, figma files). By folding in or merging, there is a larger surface area for litigation in the OSD repo. At least by forking, these design files are not part of the same OSD codebase.

Fourthly, and more generically to the third point, and IANAL, but in the spirit of software licensing enforcement, using external compiled binary packages are always a safer route than copying code. By folding in or merging, we are copying EUI code into OSD, and any files that were accidentally/intentionally not Apache license in EUI will mess up the entire OSD codebase.

kavilla commented 2 years ago

First, doesn't folding in and merging also mean that all git history will be lost? The history helps a great deal when understanding certain changes during maintenance. Forking doesn't have this problem.

I believe there are some ways to keep the history. But I agree we should keep make sure the solution has the history.

Thirdly, there are some design files in EUI that have had Elastic trademarks (icons, palettes, figma files). By folding in or merging, there is a larger surface area for litigation in the OSD repo. At least by forking, these design files are not part of the same OSD codebase.

Ideally part of the process would be replacing these trademark and the version we merged has files that are majority, if not all, AL2 licensed. However, I do agree, the previous maintainers had no problem mixing the licenses.

dblock commented 2 years ago

If we're considering doing a ton of work, forking and such, we should also consider replacing the entire UI with another framework, e.g. https://github.com/opensearch-project/OpenSearch-Dashboards/issues/1228.

AMoo-Miki commented 2 years ago

After getting a branch ready with folded-in EUI and Charts, I have started to dislike the idea because it adds over 10 minutes to the almost quick bootstrapping process.

There are 3 options: 1) Live with the additional 10+ mins, 2) Abandon folding in and just fork, or 3) Commit a prebuilt EUI to the repo, just like osd-pm.

AMoo-Miki commented 2 years ago

If we're considering doing a ton of work, forking and such, we should also consider replacing the entire UI with another framework, e.g. #1228.

Forking EUI and dealing with the incompatibilities and vulnerabilities would need a lot less effort when compared to pulling in a new UI framework, dealing with incompatibilities and vulnerabilities, and filling in missing components and features.

AMoo-Miki commented 2 years ago

After getting a branch ready with folded-in EUI and Charts, I have started to dislike the idea because it adds over 10 minutes to the almost quick bootstrapping process.

There are 3 options:

1. Live with the additional 10+ mins,

2. Abandon folding in and just fork, or

3. Commit a prebuilt EUI to the repo, just like `osd-pm`.

On my dev environment, starting from a clean slate and excluding the time for installing the extra dependencies, bootstrapping Charts adds roughly 80s and EUI adds about 500s.

The biggest chunks are from building ES modules (70s), CommonJS modules (95s), test environment (65s), declaration files (40s), regular bundle (70s), and minified bundle (95s).

tmarkley commented 2 years ago

@AMoo-Miki Thanks for those details. I think that paired with the messy process of inheriting the entire commit history in OpenSearch Dashboards (would that be reversible? I've never tried something like that before) should push us to more seriously consider option 2 (forking). Separation of concerns, community contribution practices/standards (i.e. difference between EUI and Dashboards), and future ownership are significant factors as well, some of which were brought up in comments above.

AMoo-Miki commented 2 years ago

Following a debate, the consensus reached is to fast-forward or redo the bump PR and separately fork, rename, and fix EUI and Charts.