[Fleet] Make Elasticsearch scope setting a select #5451

jillguyonnet · 2023-03-06T16:20:47Z

What does this PR do?

This PR changes the field type of the scope setting for the Elasticsearch integration from text to select. The select type is new and defined in the following PRs:

Checklist

I have reviewed tips for building integrations and this pull request is aligned with them.
I have verified that all data streams collect metrics or logs.
I have added an entry to my package's changelog.yml file.
I have verified that Kibana version constraints are current according to guidelines.

Author's Checklist

New scope select variable works as expected.
The label text for scope is approved.
Changes to tests are approved.

How to test this PR locally

Build this package and run with Kibana 8.8.0.
Check that scope option works as desired.

Related issues

Closes [Change Proposal] Add support for "select/option" type for Integration parameters package-spec#453

Screenshots

Cf. screenshots in elastic/kibana#152550

elasticmachine · 2023-03-06T16:32:15Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-03-31T16:24:48.734+0000
Duration: 31 min 25 sec

Test stats 🧪

Test	Results
Failed	0
Passed	60
Skipped	0
Total	60

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2023-03-24T10:28:42Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`5/5`)	💚
Files	100.0% (`9/9`)	💚
Classes	100.0% (`9/9`)	💚
Methods	87.5% (`98/112`)	👍
Lines	91.98% (`562/611`)	👍
Conditionals	100.0% (`0/0`)	💚

jillguyonnet · 2023-03-27T10:55:44Z

/test

jillguyonnet · 2023-03-27T16:28:39Z

@elastic/infra-monitoring-ui Could we please get your eyes on this change? This is an update to the Elasticsearch integration using the new select variable described in elastic/package-spec#453. In particular, we'd appreciate your input on the following:

As we had to bump format_version to make use of the latest package-spec version, the following tests broke. We tried to fix them to the best of our knowledge, but are not familiar enough with it.
- Pipeline and system tests for slowlog data stream
- Static tests for enrich and ccr data streams
With the change of variable type, the description text should likely be amended. Removing the first sentence of it is only a suggestion.

jlind23 · 2023-03-28T13:34:01Z

@weltenwort can we please have your eyes on this?

klacabane · 2023-03-28T14:47:30Z

packages/elasticsearch/data_stream/enrich/sample_event.json

@@ -45,7 +45,7 @@
    "data_stream": {
        "namespace": "default",
        "type": "metrics",
-        "dataset": "elasticsearch.enrich"
+        "dataset": "elasticsearch.stack_monitoring.enrich"


These sample events were generated before we specified an override for the dataset and I suppose elastic-package now validates the field.
The slowlogs failures should be related to that new validation: the spec expects the raw datastream name elasticsearch.slowlog but log4j configuration specifies an override outside of the package scope, causing a disconnect

packages/elasticsearch/data_stream/ingest_pipeline/fields/fields.yml

klacabane · 2023-03-28T14:53:14Z

packages/elasticsearch/data_stream/slowlog/_dev/test/system/test-indexing-default-config.yml

@@ -0,0 +1,5 @@
+input: logfile


Do we need to split the tests ? system tests are already expensive to run in this package given the number of data streams, will this create an additional test loop with a service up setup/teardown ?

We are not sure about this. The concern we had was that replacing elasticsearch.index_search_slowlog and elasticsearch.index_indexing_slowlog with elasticsearch.slowlog in log4j2 will change the testing behaviour, as everything would point to one dataset instead of two.

klacabane · 2023-03-28T15:23:03Z

The dropdown shows an error because no option is selected, but the first option is actually picked automatically. Should that first option be the implicit default ?

jillguyonnet · 2023-03-28T16:40:59Z

The dropdown shows an error because no option is selected, but the first option is actually picked automatically. Should that first option be the implicit default ?

Hey @klacabane Thanks for catching this! This looks like an issue with the support in Kibana, which I will tackle first thing tomorrow morning. I will ping you on this PR when this is fixed 🙏

packages/elasticsearch/manifest.yml

## Summary This PR is a followup fix to #152550. As discovered in elastic/integrations#5451 (comment), there is currently an issue where the select is invalid upon first render because no default `value` exists, yet the first option appears to be selected. The fix introduced in this PR replaces the EUISelect with a more flexible EUIComboBox component, with a placeholder with text `Select an option` when no value is selected. In addition, this new component allows the selection to be cleared, which can be useful for non required variables. If the package specifies a default value, it should be selected by default. Otherwise, the placeholder will render. Note that the field will not be valid upon first render if the variable is required and no default value is provided. <img width="1728" alt="Screenshot 2023-03-29 at 17 47 28" src="https://user-images.githubusercontent.com/23701614/228596370-d6b14c03-4a09-4f68-a137-95bb7ca7e78f.png"> <img width="1728" alt="Screenshot 2023-03-29 at 17 47 35" src="https://user-images.githubusercontent.com/23701614/228596387-dc99e4b2-5d9d-4218-baf1-010bf527b028.png"> <img width="1728" alt="Screenshot 2023-03-29 at 17 47 44" src="https://user-images.githubusercontent.com/23701614/228596415-9f1f73af-a9f5-4a88-a700-999213500c4e.png"> ### Repro steps 1. Run package-registry with version 1.5.0 of Elasticsearch integration from elastic/integrations#5451: 1. In integrations repo, check out the branch in elastic/integrations#5451 2. In `packages/elasticsearch/changelog.yml`, change `version: 1.5.0-next` to `version: 1.5.0` 3. In `packages/elasticsearch/manifest.yml`, change the version to 1.5.0 4. In the shell, `cd packages/elasticsearch` if not there already and `elastic-package build -v` 5. Run the package registry: `elastic-package stack up -v -d --services package-registry` 6. Check that it is running correctly and that you can see version 1.5.0 of Elasticsearch at https://localhost:8080/search?package=elasticsearch 2. Run Kibana in dev on this branch: 1. In your `kibana.dev.yml`, add `xpack.fleet.registryUrl: https://localhost:8080` if not there (⚠️ https, not http) 2. Before running `yarn start`, run `export NODE_EXTRA_CA_CERTS=$HOME/.elastic-package/profiles/default/certs/kibana/ca-cert.pem` in the same shell. This is to set up the certificate needed to access EPR with https. 3. In Kibana, add the Elasticsearch integration (should be version 1.5.0). Check that the `Scope` setting is controlled by a select. Note: in the latest version, this integration now provides a default value, which should be set to `node`. ### Checklist - [ ] 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) - [ ] [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)) - [ ] 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)

jillguyonnet · 2023-03-30T16:03:28Z

/test

jillguyonnet · 2023-03-31T08:22:52Z

/test

jillguyonnet · 2023-03-31T09:16:12Z

Hey @klacabane just a note that the Kibana fix has been merged - you should be able to test manually again. I'm looking at these test failures.

Separate system tests for slowlog datastream to check independently logs from index_indexing_slowlog and index_search_slowlog files. Updated log4j2.properties files to keep dataset field with the required pattern.

klacabane

LGTM, thank you for the change and taking care of test failures

elasticmachine · 2023-04-03T14:19:43Z

Package elasticsearch - 1.6.0 containing this change is available at https://epr.elastic.co/search?package=elasticsearch

elasticmachine · 2023-04-03T14:19:45Z

Package elasticsearch - 1.6.0 containing this change is available at https://epr.elastic.co/search?package=elasticsearch

jillguyonnet added the enhancement New feature or request label Mar 6, 2023

jillguyonnet mentioned this pull request Mar 7, 2023

[Change Proposal] Add support for "select/option" type for Integration parameters elastic/package-spec#453

Closed

jillguyonnet marked this pull request as ready for review March 27, 2023 16:19

jillguyonnet requested a review from a team as a code owner March 27, 2023 16:19

klacabane reviewed Mar 28, 2023

View reviewed changes

packages/elasticsearch/data_stream/ingest_pipeline/fields/fields.yml Show resolved Hide resolved

klacabane reviewed Mar 28, 2023

View reviewed changes

jillguyonnet mentioned this pull request Mar 29, 2023

[Fleet] Add placeholder to integration select field elastic/kibana#153927

Merged

6 tasks

klacabane reviewed Mar 30, 2023

View reviewed changes

packages/elasticsearch/manifest.yml Outdated Show resolved Hide resolved

jillguyonnet and others added 10 commits March 31, 2023 17:59

[Fleet] Make Elasticsearch scope setting a select

69795c8

Constrain Kibana to 8.8.0 and higher

7847da8

Fix package linting

e74038a

Add changelog entry

6475bba

Fix tests

9aca07e

Fix static tests

8d28e7d

Add missing build changes

c640092

Separate system tests for slowlog

fe2c888

Separate system tests for slowlog datastream to check independently logs from index_indexing_slowlog and index_search_slowlog files. Updated log4j2.properties files to keep dataset field with the required pattern.

Set default value to node

b2216f2

Add node.roles

c67f125

jillguyonnet added 2 commits March 31, 2023 18:17

Amend README

cd7646f

Rebuild README

8ae90f0

klacabane approved these changes Apr 3, 2023

View reviewed changes

jillguyonnet merged commit c0dc29d into elastic:main Apr 3, 2023

jillguyonnet deleted the fleet/Make-Elasticsearch-scope-setting-select branch April 3, 2023 13:59

[Fleet] Make Elasticsearch scope setting a select #5451

[Fleet] Make Elasticsearch scope setting a select #5451

Uh oh!

Conversation

jillguyonnet commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Checklist

Author's Checklist

How to test this PR locally

Related issues

Screenshots

Uh oh!

elasticmachine commented Mar 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

jillguyonnet commented Mar 27, 2023

Uh oh!

jillguyonnet commented Mar 27, 2023

Uh oh!

jlind23 commented Mar 28, 2023

Uh oh!

klacabane Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

klacabane Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

jillguyonnet Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

klacabane commented Mar 28, 2023

Uh oh!

jillguyonnet commented Mar 28, 2023

Uh oh!

Uh oh!

jillguyonnet commented Mar 30, 2023

Uh oh!

jillguyonnet commented Mar 31, 2023

Uh oh!

jillguyonnet commented Mar 31, 2023

Uh oh!

klacabane left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 3, 2023

Uh oh!

elasticmachine commented Apr 3, 2023

Uh oh!

Uh oh!

jillguyonnet commented Mar 6, 2023 •

edited

Loading

elasticmachine commented Mar 6, 2023 •

edited

Loading

elasticmachine commented Mar 24, 2023 •

edited

Loading