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

Support for custom geojson via region map #1408

Closed
2 tasks
Shivamdhar opened this issue Mar 30, 2022 · 18 comments
Closed
2 tasks

Support for custom geojson via region map #1408

Shivamdhar opened this issue Mar 30, 2022 · 18 comments
Labels
enhancement New feature or request

Comments

@Shivamdhar
Copy link
Contributor

Shivamdhar commented Mar 30, 2022

Is your feature request related to a problem? Please describe.

opensearch-project/geospatial#122
Currently, Opensearch provides a standard set of GeoJSONs to its customers. This task extends the scope by letting users to configure custom GeoJSONs for adding specific vector layers for visualization.

Describe the solution you'd like

Extend the region map plugin to include:

  • a new tab to allow users to import a custom map that integrates with the backend geospatial plugin
  • ability to select the custom maps uploaded and make use of them via Layer Options tab for visualization

This can be achieved by completing the following tasks:

  • Allow users to upload custom vector map
  • Allow users to select index as custom vector map for visualization

Describe alternatives you've considered

Since this feature was to be bundled as part of core OSD as an extension of region map, there was no other alternative considered for the time being.

Additional context

Screen Shot 2022-03-30 at 4 06 11 PM

@Shivamdhar Shivamdhar added the enhancement New feature or request label Mar 30, 2022
@Shivamdhar Shivamdhar changed the title Support for custom geojson via region map plugin Support for custom geojson via region map Mar 30, 2022
@Shivamdhar
Copy link
Contributor Author

I'll be taking this up, wanted to check if we should raise PRs to main or create a feature branch for all the changes required with regards to this task.

@VijayanB
Copy link
Member

@kavilla @boktorbb-amzn Any suggestions? Thanks

@kavilla
Copy link
Member

kavilla commented Mar 31, 2022

Hello @Shivamdhar, @VijayanB,

Sounds awesome and exciting!

Couple things. Will there be a follow up on the design and/or documentation of the APIs? How OpenSearch Dashboards will interact with these new APIs or has that been published I can't seem to find something (didn't look too far though).

My biggest worry about this is about features being inserted into a core OpenSearch Dashboards plugin while relying an OpenSearch plugin that is not part of the core plugins over there. This will have a lot of implications. The most important one is that any community members or forks that do not consume the full distribution will have a plugin feature that won't work. The legacy application did have some nested 'x-pack' code in OSS plugins and it made the logic quite messy and added a lot of technical debt. So if we do end up nesting this into the region map plugin the verification that it takes to ensure that plugin won't be available if the OpenSearch plugin is not available might be more work than just creating an OpenSearch Dashboards external plugin like the one for OpenSearch. In fact, we can generate the plugin and store the OpenSearch Dashboards plugin in https://github.com/opensearch-project/geospatial. For example, like https://github.com/opensearch-project/dashboards-reports is a hybrid repo of OpenSearch and OpenSearch Dashboards.

However, if it is a requirement to put in the OpenSearch Dashboards repo then I will like to see a big push to place the https://github.com/opensearch-project/geospatial into the OpenSearch repo.

I think we should do an RFC or convert this one to. Explicitly laying the proposal if we have committed a decision and then if you can link a design to the community about how the full plugin works from front to back that will be great to share!

Excited to see what comes!

cc: @seanneumann, @dblock, @nknize (I believe Nick might be overqualified for stuff related to geospatial)

@Shivamdhar
Copy link
Contributor Author

Thanks @kavilla for your feedback.

RFC can be accessed here: opensearch-project/geospatial#47

Following has been confirmed -

  • Geospatial plugin won't be part of core at the moment.
  • Support for custom GeoJSON will be added in OpenSearch Dashboards repo for now.

In order to run OpenSearch Dashboards independent of geospatial plugin (in the instance when geospatial plugin is not installed), proper messaging will be shown to the users regarding the same.

@tmarkley
Copy link
Contributor

I share the same sentiment as @kavilla on this. Modifying core plugins/features/configurations to accommodate this one feature is not a maintainable approach.

@ashwin-pc
Copy link
Member

@Shivamdhar, Love the feature add but i dont think i fully understand why this need to be a part of core OpenSearch Dashboards.

What would happen if the user installs only the main distributions of OpenSearch Dashboards and OpenSearch Core? Looking at this issue and the comments, it looks like this feature relies on a plugin that does not come out of the box for OpenSearch.

If it does need the OpenSearch geospatial plugin, why is including this as an OpenSearch Dashboards plugin too not an option?

The RFC you linked was in a different repository even though it affects OpenSearch Dashboards. It also has no comments on it. It might be worthwhile to have this RFC open on this repo to gather feedback from the community. We can also convert this issue into the RFC since we already have some momentum on it.

I dont disagree with the need for this feature, i'm just not sure this is the right place to introduce it. Maybe i'm just missing something here. Would love to hear your thoughts.

@alisazosimova
Copy link

Hi dear OS developers!

Is there a time line where this can be available for the public?

@vamshin
Copy link
Member

vamshin commented Apr 27, 2022

@alisazosimova We are targetting for 2.1 release scheduled for June 30.
You can see the roadmap here https://github.com/orgs/opensearch-project/projects/1

@Shivamdhar
Copy link
Contributor Author

I share the same sentiment as @kavilla on this. Modifying core plugins/features/configurations to accommodate this one feature is not a maintainable approach.

Hey @tmarkley, thanks for looking into this issue. Bootstrapping as a new plugin just to add a new tab in the current region map visualization tab will be an overkill at this point. However, our long term goal to revamp all maps including region map into a separate plugin (opensearch-project/dashboards-maps#15) is in place like you mentioned. Hence, we believe creating a new plugin just only for this feature will not be beneficial for us.

What would happen if the user installs only the main distributions of OpenSearch Dashboards and OpenSearch Core? Looking at this issue and the comments, it looks like this feature relies on a plugin that does not come out of the box for OpenSearch.

Hey @ashwin-pc, thank you for your feedback.
If the user installs the minimum distribution, they will not be able to access the custom GeoJSON upload tab and there will be a prompt that will direct them to install geospatial plugin in order to enable the feature.
Screen Shot 2022-04-27 at 12 05 01 PM

The RFC you linked was in a different repository even though it affects OpenSearch Dashboards. It also has no comments on it. It might be worthwhile to have this RFC open on this repo to gather feedback from the community. We can also convert this issue into the RFC since we already have some momentum on it.

The RFC is part of geospatial repo as it has the backend API. We could consider converting this issue to RFC as well : )

Please let us know if there are any more concerns.

@ashwin-pc
Copy link
Member

@Shivamdhar This feels very bad from a CX perspective. It feels incomplete to have a message like this out of the box when the user installs the minimum distribution of OS core and Dashboards. Bootstrapping a plugin, especially since you already have one for core should not be any more expensive than trying to merge it into Dashboards. If it is, thats probably a gap we should close rather than taking the shorthand approach of adding it into core.

Also since you mentioned that you intend on revamping maps in a separate plugin, why not start with this feature and then extend it to be what you envision for the future.

@vamshin
Copy link
Member

vamshin commented Apr 28, 2022

@ashwin-pc as called out by Shivam, bootstapping plugin for adding a tab is over kill. Also this new feature is so integrated that we need to pull whole of regional maps and coordinate maps to the new plugin along with this feature which is a huge effort and potentially causing breaking issues. Since we anyways have plans to merge regional and coordinate maps to single Maps dashboard, it makes sense to bundle this effort part of merge feature.

Also >90% of our downloads are docker images which bundle all the plugins together where issue wont even exist. Considering this is blocking community and considering the magnitude of change(new tab and a file upload box), we would like to push bootstrapping effort to subsequent releases.

@tmarkley @kavilla @ashwin-pc please help us review this feature. Created issue to bootstrap new plugin for our future release where we merge both coordinate and regional maps opensearch-project/maps#14

Also Maps team would be taking ownership on any issues related to this feature.

@seanneumann
Copy link
Contributor

@vamshin from my perspective, this doesn't make sense to be included in core Dashboards. We need to think about what makes sense as a minimum distribution. This will be extra important as we decouple Dashboards from OpenSearch in the future and create more capabilities to dynamically load in new plugins.

Can you add this functionality to the maps plugin?

@vamshin
Copy link
Member

vamshin commented Apr 28, 2022

@seanneumann, we have maps both regional maps and coordinate maps sitting on core Dashboards since the inception of opendistro and opensearch. What we will be doing here is adding a tab that uploads a file and make it available in the existing regional maps.

This new feature is so integrated that we need to pull whole of regional maps and coordinate maps to the new plugin which is a huge effort and most importantly opening doors for potentially causes breaking issues. Since we anyways have plans to revamp Maps(combine regional and coordinate maps into layers), our proposal is to have a new dashboard-maps plugin then and move the code.

This is blocker for community adopting Maps. Please help us accept the changes and we are preparing grounds for brand new dashboard-maps plugin for our future proposals.

@tmarkley
Copy link
Contributor

Also >90% of our downloads are docker images which bundle all the plugins together where issue wont even exist. Considering this is blocking community and considering the magnitude of change(new tab and a file upload box), we would like to push bootstrapping effort to subsequent releases.

Does this number truly capture all customers/users?

This new feature is so integrated that we need to pull whole of regional maps and coordinate maps to the new plugin which is a huge effort and most importantly opening doors for potentially causes breaking issues. Since we anyways have plans to revamp Maps(combine regional and coordinate maps into layers), our proposal is to have a new dashboard-maps plugin then and move the code.

It's a bit unclear what the rush is to merge this in then if there is a better solution planned?

Also Maps team would be taking ownership on any issues related to this feature.

It's still in our repository so we are by default taking co-ownership of this feature. We want what's best for the community but we need to take maintainability into consideration.

@ahopp
Copy link
Contributor

ahopp commented Apr 29, 2022

I have a strong bias for not modifying more plugins/features/configurations to accommodate in this case. I particularly don't find "doing it another way is hard" or "if we do it poorly it can break things" to be compelling justifications. Additionally, while I understand your bias for speed, I am strongly resistant to rushing this in, as a solution the maintainers of the Dashboards repo are opposed to, if there is a better solution planned. Since you have already indicated your long-term plan is to revamp Maps (combine regional and coordinate maps into layers), why would we incorporate into Dashboards now and then remediate later?

Also Maps team would be taking ownership on any issues related to this feature.

Simply put, that's not at all how repo ownership or responsibility works. Regardless of the contributors, the Dashboards maintainers will need to speak to, own, and support any code in the repo until such point it is no longer in the Dashboards repo. We absolutely cannot pick and choice code ownership within a repo in the manner in which your statement implies.

This is blocker for community adopting Maps.

Can you highlight some examples of the community being blocked by this code being implemented in the Maps? I'm not sure I understand how this would block. OR do you mean, "we should push it in now so users can use it now rather than waiting for us to implement it as planned (i.e., revamp Maps)"? I haven't seen the OpenSearch community raising flags here as a critical blocker, but I might have missed those issues - can you share?

Finally, you say that the long-term plan is to revamp Maps, but I haven't seen this as part of an RFC - can you share this as well? I'd like to catch up on the context of that historical decision. I don't see a Dashboards RFC for this short-term solution and the the long-term "plan" issue you linked above seems to have only a 1 sentence feature request by you. There is a maps RFC, but this hasn't be reviewed or commented on and doesn't really cover the Dashboards implications.

Like others, I don't think this is a matter or disagreement on the user need - I'm aligned on and understand why we want to build this for the community...I just don't understand why we are introducing it in this manner.

@vamshin
Copy link
Member

vamshin commented May 2, 2022

@ahopp I think we are mixing multiple philosophies here :). I will try my best to narrow down the scope of this feature and lets take another stab to see if this fits to be a dedicated plugin on its own. To keep it simple, lets remove revamp of Maps out of equation.

What are we adding now?

We are developing a feature that extends existing feature which has been part of the dashboards plugin since the inception of Maps. Precisely we are adding a new tab to let the users upload custom vector maps to the already existing vector maps
Given the above scope lets precisely think if this warrants to be a dedicated plugin or provides motivation for creating dedicated plugin.

Can you highlight some examples of community being blocked by this code being implemented in the Maps?

https://forum.opensearch.org/t/how-to-add-layers-to-a-map-in-opensearch-dashboard-1-2-0/8654
https://forum.opensearch.org/t/status-of-kibana-maps/5106/14
https://forum.opensearch.org/t/create-maps-on-opendistro/7188
Now that we have some view into the community interests. Lets think backward from community needs and see if this particular feature warrants to be a dedicated plugin and there by making community wait for 3.0 launch scheduled on Jan 2023(timeline to revamp maps part of ability to add multiple layers).

Finally, you say that the long-term plan is to revamp Maps, but I haven't seen this as part of an RFC - can you share this as well?

Thanks for your interest in Maps. We are working on RFC and planning to publish soon. Let me try to explain the idea here. If you have some experience looking at Maps dashboard you would see there are two kinds of maps (I)Regional Maps and (2)Coordinate Maps. Our plan is to merge both of them to single Maps layout and have ability for customers to add layers. As mentioned this is a major refactor and thats where we plan to move it as a dedicated maps plugin.

@kavilla
Copy link
Member

kavilla commented Jun 2, 2022

#1632 enables for extending.

Will close.

@manudipko
Copy link

when might this feature be available on AWS OpenSearch Dashboards?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

10 participants