-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Console] Generate autocomplete definitions from ES specification #163301
[Console] Generate autocomplete definitions from ES specification #163301
Conversation
availability.stack = endpoint.availability.stack?.visibility === 'public'; | ||
// availability is a required property of the endpoint | ||
if (!endpoint.availability) { | ||
throw new Error('missing availability for ' + endpoint.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwing an error here because the script expects all endpoints to have availability
according to types. An error here will indicate that something changed in the schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a test to expect this scenario to throw? 🤔 I've checked the .test.ts file but couldnt find any for this scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will add a test for this!
@@ -47,7 +47,7 @@ const convertValueOf = ( | |||
return convertLiteralValue(valueOf); | |||
} | |||
// for query params we can ignore 'dictionary_of' and 'user_defined_value' | |||
return ''; | |||
throw new Error('unexpected valueOf type ' + kind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another safeguard: the script doesn't expect types like dictionary
and literal value
for url params. an error here will indicate that an update is needed.
@@ -124,6 +124,8 @@ export function setActiveApi(api) { | |||
dataType: 'json', // disable automatic guessing | |||
headers: { | |||
'kbn-xsrf': 'kibana', | |||
// workaround for serverless | |||
'x-elastic-internal-origin': 'Kibana', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a workaround to fix #163318, needs to be refactored to the Kibana http client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this isn't the preferred approach, but just to confirm - are there any particular concerns with moving forward with this workaround for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No concerns, I confirmed with the Core team that adding the header is one way to fix this issue. There are more details in this PR. My only concern is that we use jQuery ajax for this request 😓
Pinging @elastic/platform-deployment-management (Team:Deployment Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @yuliacech 🎉 Left a few questions in the code about some of the generated definitions.
I did some smoke testing locally. Everything seemed to work as expected for stateful. For serverless, I am seeing some autocomplete suggestions that I would not expect - I think these are coming from the overrides
directory.
@@ -124,6 +124,8 @@ export function setActiveApi(api) { | |||
dataType: 'json', // disable automatic guessing | |||
headers: { | |||
'kbn-xsrf': 'kibana', | |||
// workaround for serverless | |||
'x-elastic-internal-origin': 'Kibana', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this isn't the preferred approach, but just to confirm - are there any particular concerns with moving forward with this workaround for now?
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/{branch}/cluster-stats.html", | ||
"availability": { | ||
"stack": true, | ||
"serverless": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this one is public in serverless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this needs to be updated in the spec, it's marked as public
for serverless there
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/master/indices-templates.html", | ||
"availability": { | ||
"stack": true, | ||
"serverless": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for legacy index templates, right? I thought that was going to be blocked for serverless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs update in the spec too, marked as public for serverless
"methods": [ | ||
"POST" | ||
], | ||
"patterns": [ | ||
"_ml/anomaly_detectors/_validate" | ||
], | ||
"documentation": "https://www.elastic.co/guide/en/machine-learning/current/ml-jobs.html" | ||
"documentation": "https://www.elastic.co/guide/en/machine-learning/current/ml-jobs.html", | ||
"availability": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean if the API availability is false
in both environments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually that is either an internal endpoint like for example _internal.delete_desired_balance
, or it's still under a feature flag, or in this case it looks like it's a deprecated/moved/renamed endpoint. I was not able to find this one in the docs pages (link).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. It sounds like this one may also need to be updated in the specs then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think this one is probably moved to a different endpoint or deprecated completely. This should be updated in the spec.
}, | ||
"methods": [ | ||
"GET" | ||
], | ||
"patterns": [ | ||
"_application/search_application" | ||
], | ||
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/master/list-search-applications.html" | ||
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/master/list-search-applications.html", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a concept of APIs specific to a serverless project type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, I asked about that the ES team but they said for now it's just public vs internal vs blocked in serverless. And the ES spec only has stack
vs serverless
availability
"documentation": "https://www.elastic.co/guide/en/elasticsearch/reference/master/indices-disk-usage.html", | ||
"availability": { | ||
"stack": true, | ||
"serverless": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this one is public too on serverless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this one is "internal" in the sheet but not yet updated in the spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @yuliacech! Did small bit of local testing and autocompletion seems to be correct. Left one small comment, let me know!
availability.stack = endpoint.availability.stack?.visibility === 'public'; | ||
// availability is a required property of the endpoint | ||
if (!endpoint.availability) { | ||
throw new Error('missing availability for ' + endpoint.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a test to expect this scenario to throw? 🤔 I've checked the .test.ts file but couldnt find any for this scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest LGTM. Thanks for addressing my feedback!
Thanks a lot for the reviews, @alisonelizabeth @sabarasaba! |
There is a PR open in the ES specification repo that updates the availability in serverless. After this PR is merged we could re-generate the definitions some time in the future when the spec PR is merged and the changes are propagated to the schema. This should be easy to do with the script and the changes will probably not be too extensive. |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
…63301) ## Summary This PR uses the new script to generate autocomplete definitions for Dev Tools Console from the ES specification repo. #### Definitions changes - New property `availability` is added to filter out endpoints that are not available in Serverless - Some endpoints' query parameters have more details now, for example common query params are now defined in definitions ```json "url_params": { "error_trace": "__flag__", "filter_path": [], "human": "__flag__", "pretty": "__flag__" }, ``` - Url components in few endpoints are removed, but those were added to overrides files in #163096 - Documentation links contain `{branch}` instead of `master` (fix for that added in #159241) #### Script changes - The logic for generating `availability` for endpoint has been updated based on the feedback from the Clients team. Details added to the script file. - Added a few "safe guards" to the spots in the script where an unexpected type of data might be coming from the ES specification schema #### Console changes - Fixed the autocomplete request on Serverless (we might need a proper fix for that, details in #163318) Also updates to readme files both in Console and the new script. I will remove the old script in a separate PR. ## Screenshots "ILM" autocomplete suggestions displayed on stateful <img width="583" alt="Screenshot 2023-08-07 at 17 47 48" src="https://github.com/elastic/kibana/assets/6585477/641a48b0-fb1a-4d3b-a8c9-99eab8795510"> "ILM" autocomplete suggestions not displayed on serverless <img width="572" alt="Screenshot 2023-08-07 at 17 35 16" src="https://github.com/elastic/kibana/assets/6585477/a1ee5468-eb9f-4f52-81d5-c661b06f8ceb"> ## How to test - Start Kibana on stateful (`yarn start`) and check that autocomplete suggestions are working as before (no changes) - Start Kibana on serverless (`yarn start --serverless`) and check that autocomplete suggestions are not displayed for endpoints blocked/internal on serverless. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
Summary
This PR uses the new script to generate autocomplete definitions for Dev Tools Console from the ES specification repo.
Definitions changes
availability
is added to filter out endpoints that are not available in Serverless{branch}
instead ofmaster
(fix for that added in [Console] Use ES specification for autocomplete definitions #159241)Script changes
availability
for endpoint has been updated based on the feedback from the Clients team. Details added to the script file.Console changes
Also updates to readme files both in Console and the new script.
I will remove the old script in a separate PR.
Screenshots
"ILM" autocomplete suggestions displayed on stateful

"ILM" autocomplete suggestions not displayed on serverless

How to test
yarn start
) and check that autocomplete suggestions are working as before (no changes)yarn start --serverless
) and check that autocomplete suggestions are not displayed for endpoints blocked/internal on serverless.Checklist
Delete any items that are not applicable to this PR.