OpenSearch: Add config button to detect opensearch version #153

iwysiu · 2023-04-24T22:06:12Z

What this PR does / why we need it:
Currently, opensearch uses a drop down menu in the config form to get the version. This is inconvenient for users and requires us to maintain the list of OpenSearch versions (which we aren't really doing). This commit exchanges the menu for a button that saves the config (to make sure it connects properly) and detects the version. It's behind the opensearchDetectVersion feature flag.

Before:

The new ui:

When the call to get the version fails:

It was also changed to default to a null version (so the user has to set it to successfully configure the datasource) and for test to error when the elasticsearch version is >= 7.11.0. I can remove those changes if we think they're unnecessary.

Error for a too high elasticsearch version:

Which issue(s) this PR fixes:

Fixes #132 #131

Special notes for your reviewer:
If you want to test this with elasticsearch, you can run make devenv sources=elastic in the main grafana repo to connect to http://localhost:9200/ or make devenv sources=elastic elastic_version=7.10.0 to pick a specific version. (Run make devenv-down to take them down after)

github-actions · 2023-04-24T22:09:32Z

Backend code coverage report for PR #153
No changes

github-actions · 2023-04-24T22:09:33Z

Frontend code coverage report for PR #153

Plugin	Main	PR	Difference
src	77.77%	77.82%	.05%

github-actions · 2023-04-24T22:10:53Z

Levitate is-compatible report:

🔍 Resolving @grafana/data@latest...
🔍 Resolving @grafana/ui@latest...
🔍 Resolving @grafana/runtime@latest...
🔍 Resolving @grafana/e2e-selectors@latest...

🔬 Checking compatibility between ./src/module.ts and @grafana/data@9.5.1...
✔ Found @grafana/data version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/ui@9.5.1...
✔ Found @grafana/ui version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/runtime@9.5.1...
✔ Found @grafana/runtime version 9.0.2 locally

🔬 Checking compatibility between ./src/module.ts and @grafana/e2e-selectors@9.5.1...
✔ Found @grafana/e2e-selectors version 9.0.2 locally

✔️ ./src/module.ts appears to be compatible with @grafana/data,@grafana/ui,@grafana/runtime,@grafana/e2e-selectors

iwysiu · 2023-04-24T22:06:49Z

src/__mocks__/openSearchDatasource.ts

@@ -0,0 +1,50 @@
+import { DataSourceInstanceSettings, DataSourcePluginMeta, PluginMetaInfo, PluginType } from '@grafana/data';


Maybe excessive? But I figured we might get use out of it in the future.

iwysiu · 2023-04-24T22:07:05Z

src/__mocks__/DefaultConfigOptions.ts

@@ -2,7 +2,9 @@ import { DataSourceSettings } from '@grafana/data';
 import { Flavor, OpenSearchOptions } from '../types';
 import { createDatasourceSettings } from '../dependencies/mocks';

-export function createDefaultConfigOptions(): DataSourceSettings<OpenSearchOptions> {


Moved this file for neatness

iwysiu · 2023-04-24T22:12:16Z

src/datasource.ts

-            message: 'OpenSearch error: ' + message,
-            error: err.data.error,
-          };
+          let newErr = new Error('OpenSearch error: ' + message);


I needed to change this to an Error type to catch it properly in OpenSearchDetails.tsx

iwysiu · 2023-04-24T22:16:41Z

src/configuration/OpenSearchDetails.tsx

+      value.jsonData.flavor} ${value.jsonData.version}`;
+  }
+
+  const getServerlessSettings = (event: React.SyntheticEvent<HTMLInputElement, Event>) => {


All of the query code assumes that a version and flavor are set, so rather than refactor that in this commit, I set the flavor and version when serverless is toggled. (I checked and you cannot get a version from serverless). I figured just setting it to the latest one was ok, as we'll need to go make other code changes anyway if a new latest version introduces breaking changes.

Do we need a version/flavor if it's serverless? Aren't all serverless the same flavor/version?

Yup, serverless are all the same! but (at least at present) our query code expects a version to be set, so we need one to be set.

I tested this with a serverless datasource and the flow as a bit weird:

Screen.Recording.2023-05-03.at.1.30.10.PM.mov

what if when we get/set version we check if it's serverless based on the url and then set the toggle?

I think we should be able to dynamically determine it based on the url, I think for example most of the endpoints are formatted like https://{someidorsomething}.{region}.aoss.amazonaws.com

I wonder if we could do a regex for aoss.amazonaws.com and if it's there we set the version to serverless or something?

iwysiu · 2023-04-25T19:31:34Z

src/configuration/ConfigEditor.tsx

+  useEffect(() => {
+    setSaved(false);
+  }, [
+    options.url,


This is probably excessive, but since the user has to click a button to save it anyway, I figured it was better to err on the side of re-saving too often.

sarahzinger

Looking cool!

I don't love the concept of having save and get version in the same button, but I also get that in this case it might make sense because you can't detect the version without saving your auth settings.

Another question that I'm only just thinking through now, is there a reason we have to surface this to users at all? What if testDatasource was responsible for getting/setting the opensearch version and we didn't show a field for this at all?

sarahzinger · 2023-04-26T13:56:25Z

src/configuration/ConfigEditor.tsx

+    }
+    await getBackendSrv()
+      .put(`/api/datasources/${options.id}`, options)
+      .then((result: { datasource: any }) => {


nitpicky but just sharing for knowledge in case its fun to rewrite async js :D that when you have await you don't need to use .thens so this could be rewritten as:

const { datasource} = await getBackendSrv().put(`/api/datasources/${options.id}`, options); options.version = darasource.version; ....

sarahzinger · 2023-04-26T13:58:52Z

src/configuration/OpenSearchDetails.test.tsx

+      expect(last(onChangeMock.mock.calls)[0].jsonData.flavor).toBe(Flavor.OpenSearch);
+      expect(last(onChangeMock.mock.calls)[0].jsonData.version).toBe('2.6.0');
+
+      // rerender with the changed value


neat I don't think I've used this before :)

sarahzinger · 2023-04-26T14:25:38Z

src/configuration/OpenSearchDetails.test.tsx

@@ -143,10 +171,15 @@ describe('OpenSearchDetails', () => {
    const onChangeMock = jest.fn();

    const defaultConfig = createDefaultConfigOptions();
+    const opensearchDetectVersionValue = config.featureToggles.opensearchDetectVersion;


might make sense to take this out of the describe, it's very unlikely but I guess I could see a world where we edit config.featureToggles.opesearchDetectVersion directly in a previous test up above and accidentally never reset it or something?

sarahzinger · 2023-04-26T14:29:24Z

src/configuration/OpenSearchDetails.tsx

+      value.jsonData.flavor} ${value.jsonData.version}`;
+  }
+
+  const getServerlessSettings = (event: React.SyntheticEvent<HTMLInputElement, Event>) => {


Do we need a version/flavor if it's serverless? Aren't all serverless the same flavor/version?

sarahzinger · 2023-04-26T14:31:20Z

src/datasource.ts

+          ` is not supported by the OpenSearch plugin. Use the ElasticSearch plugin.`,
+      });
+    }
+    //https://grafana.com/docs/grafana/latest/datasources/elasticsearch/


nice addition! Maybe we can share a link somehow?

The commented out link was from when I was trying to add the link. I couldn't get it to render properly 😞, whatever calls testDatasource displayed the whole href block.

iwysiu

Another question that I'm only just thinking through now, is there a reason we have to surface this to users at all? What if testDatasource was responsible for getting/setting the opensearch version and we didn't show a field for this at all?

That was more or less proposal 1! I think we decided not to do that so that the users would be aware that the version has to be reset if they update their OpenSearch instance.

iwysiu · 2023-04-26T15:51:48Z

src/configuration/ConfigEditor.tsx

+    }
+    await getBackendSrv()
+      .put(`/api/datasources/${options.id}`, options)
+      .then((result: { datasource: any }) => {


iwysiu · 2023-04-26T15:53:27Z

src/configuration/OpenSearchDetails.test.tsx

@@ -143,10 +171,15 @@ describe('OpenSearchDetails', () => {
    const onChangeMock = jest.fn();

    const defaultConfig = createDefaultConfigOptions();
+    const opensearchDetectVersionValue = config.featureToggles.opensearchDetectVersion;


iwysiu · 2023-04-26T17:22:06Z

src/configuration/OpenSearchDetails.tsx

+      value.jsonData.flavor} ${value.jsonData.version}`;
+  }
+
+  const getServerlessSettings = (event: React.SyntheticEvent<HTMLInputElement, Event>) => {


Yup, serverless are all the same! but (at least at present) our query code expects a version to be set, so we need one to be set.

iwysiu · 2023-04-26T18:11:40Z

src/datasource.ts

+          ` is not supported by the OpenSearch plugin. Use the ElasticSearch plugin.`,
+      });
+    }
+    //https://grafana.com/docs/grafana/latest/datasources/elasticsearch/


The commented out link was from when I was trying to add the link. I couldn't get it to render properly 😞, whatever calls testDatasource displayed the whole href block.

kevinwcyu

nice, tested out with es and os and works for me. just a comment about providing an error message earlier about the unsupported version when the save and get version button is pressed.

kevinwcyu · 2023-05-01T17:54:56Z

src/datasource.ts

@@ -630,6 +644,15 @@ export class OpenSearchDatasource extends DataSourceApi<OpenSearchQuery, OpenSea
    return JSON.stringify(queryObj);
  }

+  async getOpenSearchVersion(): Promise<{ flavor: Flavor; version: string }> {


nit: can we export the Version interface from here and use that here?

kevinwcyu · 2023-05-02T19:11:10Z

src/configuration/OpenSearchDetails.tsx

+    setVersionErr('');
+    await saveOptions();
+    try {
+      const version = await datasource.getOpenSearchVersion();


can getOpenSearchVersion check for supported versions and throw an error if it's >=7.11.0? right now it can successfully set a version and it appears to be ok, but i don't see an error until i save and test the datasource.

iwysiu

Made Kevin's changes. Still could not figure out how to get the Alert to render with a link 😞

iwysiu · 2023-05-02T21:21:02Z

src/datasource.ts

@@ -630,6 +644,15 @@ export class OpenSearchDatasource extends DataSourceApi<OpenSearchQuery, OpenSea
    return JSON.stringify(queryObj);
  }

+  async getOpenSearchVersion(): Promise<{ flavor: Flavor; version: string }> {


iwysiu · 2023-05-02T21:21:25Z

src/configuration/OpenSearchDetails.tsx

+    setVersionErr('');
+    await saveOptions();
+    try {
+      const version = await datasource.getOpenSearchVersion();


sarahzinger

Cool stuff!

I was playing around with this and one thing I could see happening is users click Save and Get Version misread this as Get and Save and think that their version is saved as well? I do sort of wonder if we should just call it Get Version after all? Or alternatively we could save again after the version is set? Idk what do you think? Am I overthinking this? It's a tricky ui question here

Screen.Recording.2023-05-03.at.1.44.10.PM.mov

Also had an idea for optimizing the serverless flow. But also these suggestions feel like things we could explore in future issues/prs if we want to merge as is I'm down!

sarahzinger · 2023-05-03T17:28:16Z

src/configuration/OpenSearchDetails.test.tsx

 const { Select, Switch } = LegacyForms;

+const opensearchDetectVersionValue = config.featureToggles.opensearchDetectVersion;


yeah this is what I meant!

sarahzinger · 2023-05-03T17:28:38Z

src/configuration/OpenSearchDetails.test.tsx

 const { Select, Switch } = LegacyForms;

+const opensearchDetectVersionValue = config.featureToggles.opensearchDetectVersion;
+
+afterAll(() => {


does this work when it's outside of the describe? it might have to be inside? Please go ahead and tell me if I'm wrong tho haha

sarahzinger · 2023-05-03T17:33:17Z

src/configuration/OpenSearchDetails.tsx

+      value.jsonData.flavor} ${value.jsonData.version}`;
+  }
+
+  const getServerlessSettings = (event: React.SyntheticEvent<HTMLInputElement, Event>) => {


I tested this with a serverless datasource and the flow as a bit weird:

Screen.Recording.2023-05-03.at.1.30.10.PM.mov

what if when we get/set version we check if it's serverless based on the url and then set the toggle?

I think we should be able to dynamically determine it based on the url, I think for example most of the endpoints are formatted like https://{someidorsomething}.{region}.aoss.amazonaws.com

I wonder if we could do a regex for aoss.amazonaws.com and if it's there we set the version to serverless or something?

iwysiu · 2023-05-04T19:04:27Z

Edited after further discussion to:

save after getting the version
have an info box explicitly call out what happens when upgrading
moves serverless toggle above the version field, so hopefully users who need that toggle it first

katebrenner · 2023-05-08T13:32:02Z

src/datasource.test.ts

+      datasourceRequestMock.mockImplementation(() => {
+        return Promise.resolve({ data: { version: { number: '7.11.1' } } });
+      });
+      //'ElasticSearch version 7.11.1 is not supported by the OpenSearch plugin. Use the ElasticSearch plugin.'


is this comment left over from development?

iwysiu added 3 commits April 18, 2023 17:15

detect version

1f53350

Make work with serverless, fix/add tests

e9995f3

Add test

71ece24

fix spacing

baaf377

iwysiu commented Apr 24, 2023

View reviewed changes

fix test

a2bf2dc

iwysiu commented Apr 25, 2023

View reviewed changes

iwysiu added 2 commits April 25, 2023 15:46

rename file?

ba998fa

capitalize file name

d0b71bd

iwysiu marked this pull request as ready for review April 25, 2023 20:47

iwysiu requested a review from a team as a code owner April 25, 2023 20:47

iwysiu requested review from fridgepoet and kevinwcyu and removed request for a team April 25, 2023 20:47

sarahzinger reviewed Apr 26, 2023

View reviewed changes

sarah comments

f54628b

iwysiu commented Apr 26, 2023

View reviewed changes

kevinwcyu approved these changes May 2, 2023

View reviewed changes

Kevin comments

6f78d35

iwysiu commented May 2, 2023

View reviewed changes

Merge branch 'main' into iwysiu/131

3563679

sarahzinger approved these changes May 3, 2023

View reviewed changes

Editted after Catherine comments

740ec5f

remove log lines

3777ae0

kevinwcyu approved these changes May 5, 2023

View reviewed changes

fix spellcheck

e6cdf31

katebrenner reviewed May 8, 2023

View reviewed changes

katebrenner approved these changes May 8, 2023

View reviewed changes

remove dev comment

e37c294

iwysiu merged commit 8d50fcd into main May 8, 2023
5 checks passed

iwysiu deleted the iwysiu/131 branch May 8, 2023 18:20

fridgepoet mentioned this pull request Jun 2, 2023

Support for 1.2.x Opensearch versions #29

Closed

		@@ -0,0 +1,50 @@
		import { DataSourceInstanceSettings, DataSourcePluginMeta, PluginMetaInfo, PluginType } from '@grafana/data';

		const { Select, Switch } = LegacyForms;

		const opensearchDetectVersionValue = config.featureToggles.opensearchDetectVersion;

OpenSearch: Add config button to detect opensearch version #153

OpenSearch: Add config button to detect opensearch version #153

Conversation

iwysiu commented Apr 24, 2023 • edited

github-actions bot commented Apr 24, 2023

github-actions bot commented Apr 24, 2023 • edited

github-actions bot commented Apr 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu Apr 25, 2023 • edited

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu Apr 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwcyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahzinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu commented May 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwysiu commented Apr 24, 2023 •

edited

github-actions bot commented Apr 24, 2023 •

edited

github-actions bot commented Apr 24, 2023 •

edited

iwysiu Apr 26, 2023 •

edited

iwysiu Apr 25, 2023 •

edited

iwysiu Apr 26, 2023 •

edited