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.

Sign up for GitHub

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

Jump to bottom

[Onboarding] Small fixes from QA #196178

Merged
merged 2 commits into from
Oct 15, 2024
Merged

[Onboarding] Small fixes from QA #196178

merged 2 commits into from
Oct 15, 2024

Conversation

joemcelroy
Copy link
Member

@joemcelroy joemcelroy commented Oct 14, 2024 •
edited by kibanamachine
Loading

Summary

  • update the code examples to use the normal client, not the elasticsearch client. The devtools team wants us to use the elasticsearch client here
  • update the code samples highlighting component so you can see highlighting

@joemcelroy joemcelroy requested a review from a team as a code owner October 14, 2024 15:58
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
searchIndices 158.1KB 158.0KB -99.0B

joemcelroy
joemcelroy commented Oct 14, 2024
View reviewed changes
x-pack/plugins/search_indices/public/code_examples/javascript.ts
@@ -19,7 +19,7 @@ export const JAVASCRIPT_INFO: CodeLanguage = {
codeBlockLanguage: 'javascript',
};

const SERVERLESS_INSTALL_CMD = `npm install @elastic/elasticsearch-serverless`;
const SERVERLESS_INSTALL_CMD = `npm install @elastic/elasticsearch`;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we made a decision to use the elasticsearch client vs the elasticsearch-serverless client even for serverless. cc @miguelgrinberg

Copy link

@miguelgrinberg miguelgrinberg Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 👍 but we also need @pquentin and @JoshMock to comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is just for generating code snippets presented to Serverless customers, I don't see an issue. 👍

For a second I thought this was about actually using the Serverless client in the Kibana codebase, which was discussed in #169674 and is currently blocked.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just generating code snippets. It's mainly about what we should advise developers to use for interfacing with serverless. When I spoke with @miguelgrinberg, we decided that we want customers to use Elasticsearch client for both stack and serverless. Just double-checking here to ensure that this hasn't changed.

Also, it would be good to update the code exporter library with this direction. I'm hoping to use the code exporter for these examples in the future instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, just generating code snippets. It's mainly about what we should advise developers to use for interfacing with serverless. When I spoke with @miguelgrinberg, we decided that we want customers to use Elasticsearch client for both stack and serverless. Just double-checking here to ensure that this hasn't changed.

For the foreseeable future, the Serverless API will remain a subset of the Elasticsearch API, with the caveat that features usually land in Serverless weeks before they go in an Elasticsearch release. I think I'm lacking context here, but for "developers that interface with Serverless", we should likely recommend the elasticsearch-serverless client, as:

  1. it won't encourage you to use APIs that don't exist in Serverless,
  2. it will contain the APIs that are in Serverless but not in the Stack releases yet.

But it's not that simple:

  • The elasticsearch client will work on Serverless, after all. And we care less about code completion for examples.
  • I don't think the elasticsearch-serverless client supports HTTP, which means it can't be used against start-local. Likewise, it does not support like node sniffing which might be useful to use conditionally if you're migrating from a local cluster to Serverless.
  • We initially wanted to have a "serverless mode" in the elasticsearch client to restrict the APIs and parameters to the subset that worked everywhere, but since Serverless is only a subset, that was not needed.
  • There are ongoing discussions to merge the two clients, but it's unclear how we can make that work yet due to the very different release schedules.

For all those reasons, it can make sense to use the elasticsearch client here. In other words, LGTM!

Also, it would be good to update the code exporter library with this direction. I'm hoping to use the code exporter for these examples in the future instead.

What is the code exporter library? request-converter, which is used in Dev Tools to export Python and JS code, uses the elasticsearch client: https://github.com/elastic/request-converter/blob/main/src/exporters/javascript.tpl

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry yes, i meant the request-converter. Great that always uses elasticsearch client!

@joemcelroy joemcelroy added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Oct 14, 2024
efegurkan
efegurkan approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@efegurkan efegurkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joemcelroy joemcelroy merged commit 422cad5 into elastic:main Oct 15, 2024
25 checks passed
@joemcelroy joemcelroy deleted the fixes-from-qa branch October 15, 2024 10:44
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11344443606

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2024
@joemcelroy
[Onboarding] Small fixes from QA (elastic#196178)
4cc3560 
## Summary

- update the code examples to use the normal client, not the
elasticsearch client. The devtools team wants us to use the
elasticsearch client here
- update the code samples highlighting component so you can see
highlighting

(cherry picked from commit 422cad5)
@kibanamachine kibanamachine mentioned this pull request Oct 15, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine mentioned this pull request Oct 15, 2024
Merged
kibanamachine added a commit that referenced this pull request Oct 15, 2024
@kibanamachine @joemcelroy
[8.x] [Onboarding] Small fixes from QA (#196178) (#196276)
85ab195 
# Backport

This will backport the following commits from `main` to `8.x`:
- [[Onboarding] Small fixes from QA
(#196178)](#196178)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Joe
McElroy","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-15T10:44:17Z","message":"[Onboarding]
Small fixes from QA (#196178)\n\n## Summary\r\n\r\n- update the code
examples to use the normal client, not the\r\nelasticsearch client. The
devtools team wants us to use the\r\nelasticsearch client here\r\n-
update the code samples highlighting component so you can
see\r\nhighlighting","sha":"422cad5c2dca04ed121544079be255ac85f9e479","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor"],"title":"[Onboarding]
Small fixes from
QA","number":196178,"url":"https://github.com/elastic/kibana/pull/196178","mergeCommit":{"message":"[Onboarding]
Small fixes from QA (#196178)\n\n## Summary\r\n\r\n- update the code
examples to use the normal client, not the\r\nelasticsearch client. The
devtools team wants us to use the\r\nelasticsearch client here\r\n-
update the code samples highlighting component so you can
see\r\nhighlighting","sha":"422cad5c2dca04ed121544079be255ac85f9e479"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196178","number":196178,"mergeCommit":{"message":"[Onboarding]
Small fixes from QA (#196178)\n\n## Summary\r\n\r\n- update the code
examples to use the normal client, not the\r\nelasticsearch client. The
devtools team wants us to use the\r\nelasticsearch client here\r\n-
update the code samples highlighting component so you can
see\r\nhighlighting","sha":"422cad5c2dca04ed121544079be255ac85f9e479"}}]}]
BACKPORT-->

Co-authored-by: Joe McElroy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelgrinberg miguelgrinberg miguelgrinberg left review comments

@JoshMock JoshMock JoshMock left review comments

@pquentin pquentin pquentin left review comments

@efegurkan efegurkan efegurkan approved these changes

Assignees
No one assigned
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joemcelroy @elasticmachine @kibanamachine @pquentin @JoshMock @efegurkan @miguelgrinberg