[BUG] AWS paths should not be hardcoded.

[JacobBrandt]

Describe the bug

Hard coded paths like these make it impossible to reach sections of the code without using AWS.

OpenSearch-Dashboards/src/plugins/region_map/public/choropleth_layer.js

Line 196 in fb2f793

} else if (e.config.url.includes('aws.a2z.com')) {

OpenSearch-Dashboards/src/plugins/maps_legacy/public/map/opensearch_dashboards_map.js

Line 614 in fb2f793

if (baseLayer._url.includes('search-services.aws.a2z.com')) {

Solution
Remove hard coded URLs that point to AWS. Retrieve these values from https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/server/ui_settings.ts

[kavilla]

Hey @JacobBrandt, thanks for creating this issue.

However, the reason why I hardcoded these values was because these are specific code blocks for handling errors while using AWS for maps in replacement of using EMS. For the default experience we use a service provided by AWS since we could no longer use EMS, however replacing the maps service used can be replaceable from opensearch_dashboards.yml. For example,

map.tilemap.url: "https://tiles.maps.search-services.aws.a2z.com/tiles/{z}/{x}/{y}.png"
map.includeOpenSearchMapsService: false

Do we foresee us requiring access to the code blocks that are handling specific errors?

[JacobBrandt]

Yep, I understand why the values were changed and I understand that these were only used in this way to handle some edge cases. However, no section of code (even edge cases) should only be accessible when using AWS.

That's why I suggested to make this configurable. Usually settings that affect the UI go in ui_settings.ts like I mentioned. This way they can be changed from Advanced Settings and don't require a redeploy.

[kavilla]

Yep, I understand why the values were changed and I understand that these were only used in this way to handle some edge cases. However, no section of code (even edge cases) should only be accessible when using AWS.

You raise a great point! Will remove the untriaged label as this is something we should do.

[zuochengding]

Hi team,
After some investigation, here are some options in my mind. These are based on my understanding and discussion with team. Correct me if i am wrong.

1. Option 1: Remove the hardcoded error handling code directly.
   Pros: Simple and straightforward, since we no longer use aws.a2z.com as default map service end point. Instead, we use the updated opensearch.org end point from here in opensource space.
   Cons: The error handling may lose if the customer uses the customized aws end points. Need evaluate the value of the error handling?
2. Option 2: Make it configurable in a config file or service setting.
   Proposal : We can retrieve the boolean flag based on end points url we get at run time and store the value (here). then we check the value in settings.options to determine if we need handle the errors or not.
3. Option 3: Make it configurable in UI Setting.
   Proposal: Similar to option2, but allow user to configure at in Advanced Setting UI level. I have two UI proposals here:

As for options 3, I have some concerns. First of all, I personally feel that map service end point change is more lean to a service configuration and we only control the error handling (no UI difference involved). Second, if we take the value just based on user input from UI setting, there might be a chance leading to a inconsistent situation. (e.g. the user disables the error handling for aws map service setting from UI when the service points to aws end points, vice versa)

It would be appreciated if anyone can provide some input for directions.

[JacobBrandt]

Adding an Advanced Setting that mentions another company by name is also wrong for an "Open Source" repository. Why does it have to say AWS? Why can't it just say Enable Maps Service Error Logging.

[kavilla]

I agree with @JacobBrandt, shouldn't make explicit reference to the company of the mapping service even if the mapping service is owned by it since it can be changed. Technically, it is named MaRS (Maps Rendering Service).

I think if anything we should evaluate why we are attempting to catch it. If we enable the community to customize this to allow them to configure it to use their own maps service and they know it will not be on parity as OpenStreet Maps then they might desire to capture that the region's maps were blocked and have a clean error message to the user. So potentially we can re-use this as a capture region blocked error configuration and allow users to set the value that the conditional should be listening to show the pop-up if they set it. If the configuration is not set then it should just not create any region blocked exception.

Also just note, in the UI settings, that gets saved into the default index for OpenSearch Dashboards as a doc type config I believe. For multi-tenancy in the security plugin, they create multiple indices that are utilized by Dashboards. So in this case, a cluster admin who is using the OpenSearch security plugin will have to have all the tenants set this setting to get the proper warning message. But that is just a non-core plugin that we can decide to not implement this feature for, but just something to think about. But if we put this configuration in opensearch_dashboards.yml we can empower the community to utilize a region block mechanism with nice error pop-ups instead of failing with no clear indication why.

[zuochengding]

I agree with @JacobBrandt, shouldn't make explicit reference to the company of the mapping service even if the mapping service is owned by it since it can be changed. Technically, it is named MaRS (Maps Rendering Service).

I think if anything we should evaluate why we are attempting to catch it. If we enable the community to customize this to allow them to configure it to use their own maps service and they know it will not be on parity as OpenStreet Maps then they might desire to capture that the region's maps were blocked and have a clean error message to the user. So potentially we can re-use this as a capture region blocked error configuration and allow users to set the value that the conditional should be listening to show the pop-up if they set it. If the configuration is not set then it should just not create any region blocked exception.

Also just note, in the UI settings, that gets saved into the default index for OpenSearch Dashboards as a doc type config I believe. For multi-tenancy in the security plugin, they create multiple indices that are utilized by Dashboards. So in this case, a cluster admin who is using the OpenSearch security plugin will have to have all the tenants set this setting to get the proper warning message. But that is just a non-core plugin that we can decide to not implement this feature for, but just something to think about. But if we put this configuration in opensearch_dashboards.yml we can empower the community to utilize a region block mechanism with nice error pop-ups instead of failing with no clear indication why.

Totally agree with the suggestion that we shouldn't make explicit reference to the company of the mapping service.
Since this code is for map services error handling. I feel like it is enough to add a captureRegionBlockedError flag into service setting. Here is a general place to hold map service config (which will assemble both regionMap config and tileMap config). Adding the config flag to either Advanced Setting UI level or opensearch_dashboards.yml might be over kill.

The proposal:

1. Add captureRegionBlockedError flag with default value : false into maps service config.ts.
2. Expose the flag into service setting instead of ui setting.
3. Retrieve the captureRegionBlockedError from settings in the actually error handling code.

[kavilla]

Here is a general place to hold map service config (which will assemble both regionMap config and tileMap config).

Yeah if you add it here would enable people to configure this file from the opensearch_dashboards.yml file. As long as it is defined like "map" so they would be able to set it at top level. But if they fork this repo they can set the default value to whatever they like without the need of setting it the yml file.

Expose the flag into service setting instead of ui setting.

Meaning you are setting it in the config instead of inserting into the system index?

Also, do not seeing where we make the AWS path not hardcoded. Will that exist as a service setting as well?

[zuochengding]

Meaning you are setting it in the config instead of inserting into the system index?

Yes, I am planing to set a boolean flag in mapConfig, it should be stored in map service_setting

Also, do not seeing where we make the AWS path not hardcoded. Will that exist as a service setting as well?

Then in the error handling code, should something similar like this:

      baseLayer.on('tileerror', () => {
        if (settings.captureRegionError) {
          createRegionBlockedWarning();
        }
      });