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.

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

[Discover] Create Discover Shared plugin and features registry #1

Conversation

tonyghiani
Copy link
Owner

@tonyghiani tonyghiani commented Apr 9, 2024

📓 Summary

Part of elastic#180024

This implementation aims to have a stateful layer that allows the management of dependencies between Discover and other plugins, reducing the need for a direct dependency.

Although the initial thought was to have a plugin to register features for Discover, there might be other cases in future where we need to prevent cyclic dependencies.
With this in mind, I created the plugin as a more generic solution to hold stateful logic as a communication layer for Discover <-> Plugins.

Discover Shared

Based on some recurring naming in the Kibana codebase, discover_shared felt like the right place for owning these dependencies and exposing Discover functionalities to the external world.
It is initially pretty simple and only exposes a registry for the Discover features, but might be a good place to implement other upcoming concepts related to Discover.

Also, this plugin should ideally never depend on other solution plugins and keep its dependencies to a bare minimum of packages and core/data services.

flowchart TD

A(Discover) -- Get --> E[DiscoverShared]
B(Logs Explorer) -- Set --> E[DiscoverShared]
C(Security) -- Set --> E[DiscoverShared]
D(Any app) -- Set --> E[DiscoverShared]

DiscoverFeaturesService

This service initializes and exposes a strictly typed registry to allow consumer apps to register additional features and Discover and retrieve them.

The README file explains a real use case of when we'd need to use it and how to do that step-by-step.

Although it introduces a more nested folder structure, I decided to implement the service as a client-service and expose it through the plugin lifecycle methods to provide the necessary flexibility we might need:

  • We don't know yet if any of the features we register will be done on the setup/start steps, so having the registry available in both places opens the door to any case we face.
  • The service is client-only on purpose. My opinion is that if we ever need to register features such as server services or anything else, it should be scoped to a similar service dedicated for the server lifecycle and its environment.
    It should never be possible to register the ObsAIAssistant presentational component from the server, as it should not be permitted to register a server service in the client registry.
    A server DiscoverFeaturesService is not required yet for any feature, so I left it out to avoid overcomplicating the implementation.

FeaturesRegistry

To have a strictly typed utility that suggests the available features on a registry and adheres to a base contract, the registry exposed on the DiscoverFeaturesService is an instance of the FeaturesRegistry class, which implements the registration/retrieval logic such that:

  • If a feature is already registered, is not possible to override it and an error is thrown to notify the user of the collision.
  • In case we need to react to registry changes, is possible to subscribe to the registry or obtain it as an observable for more complex scenarios.

The FeaturesRegistry already takes care of the required logic for the registry, so that DiscoverFeaturesServiceis left with the responsibility of instantiating/exposing an instance and provide the set of allowed features.

Copy link

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

Looks nice overall, I just left a few questions below.

I'd like to defer judgement on the discover_shared plugin name to the DataDiscovery team.

* Retrieves all the registered features.
* @returns A Map instance.
*/
getFeaturesMap() {

Choose a reason for hiding this comment

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

I'm concerned that this (and the observable/subscription below) returns a mutable map that can influence other consumers that have received the same instance. Do we need to expose it or would register() and get() be enough?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It shouldn't be necessary at first, I'll make it private for its internal use only.

Choose a reason for hiding this comment

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

Thanks, but it still leaks out through subscribe() and observe(). Two consumers could receive the same Map and mutate it, thereby influencing each other.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You are right, I didn't think about it. Let's keep it simple as we don't have requirements for this to be observed yet, I'll change the implementation to only expose the register and getById methods.

Choose a reason for hiding this comment

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

That would solve it, thank you!

* @param id The identifier of the feature to retrieve.
* @returns The feature if found, otherwise undefined.
*/
getById(id: Feature['id']) {

Choose a reason for hiding this comment

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

Can we try to narrow the return type?

plugins/.empty Outdated

Choose a reason for hiding this comment

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

Is this removal intentional?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nope, I removed it by mistake when moving the new plugin, I'll restore it.

Choose a reason for hiding this comment

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

Nice docs ❤️

Choose a reason for hiding this comment

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

Thanks for taking the time to cleanly implement it as a service!

message: <strong>message</strong>,
logLevel: (
<strong>
{i18n.translate('xpack.logsExplorer..strong.loglevelLabel', {

Choose a reason for hiding this comment

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

Should these really be translated? If they should, there's a duplicate . in the id.

Copy link
Owner Author

Choose a reason for hiding this comment

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

They shouldn't, I didn't event noticed that because the linter auto fix it due to the recent rule @kbn/i18n/strings_should_be_translated_with_i18n. I'll change it back.

@@ -44,3 +44,4 @@ export {
} from './src';

export * from './src/types';
export * from './src/services/features_registry';

Choose a reason for hiding this comment

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

Hi @tonyghiani,

Thanks for introducing a feature registry for Discover! It's a great way to prevent circular deps.

I would suggest to create a new package for this new service and name it the same way as the new plugin. Like discover-shared or discover-registry.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @jughosta!

I considered this option as well and I've seen this pattern applied for the unified doc viewer package & plugin.
I created this on this discover utils package to avoid too many dependencies and since it's tree-shakeable, do you see any additional benefit in moving this utility into a different package?

P.S. Regarding the unified doc viewer package, it makes sense to have it in its package as it also holds the UI elements to create a more complex element and is consumed by different plugins, while this registry is ideally only used by discoverShared.

Choose a reason for hiding this comment

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

For the separation of concerns it looks cleaner to me to have a separate package.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I've implemented a new package for the registry in 5543214, let me know if looks better now :)

Copy link

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Great work @tonyghiani! This seems like it will work well for our IoC needs in One Discover, and it's great that you included documentation too. Also, I think using discover_shared as the plugin name seems reasonable and clear. And with that said, this looks good to go on my end 👍

@@ -362,6 +362,8 @@ examples/developer_examples @elastic/appex-sharedux
examples/discover_customization_examples @elastic/kibana-data-discovery
x-pack/plugins/discover_enhanced @elastic/kibana-data-discovery
src/plugins/discover @elastic/kibana-data-discovery
src/plugins/discover_shared @elastic/kibana-data-discovery @elastic/obs-ux-logs-team

Choose a reason for hiding this comment

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

++ to shared ownership 👍

@tonyghiani
Copy link
Owner Author

After further offline discussion with @jughosta , we agreed that a new package for the FeaturesRegistry class would be unnecessary as it exposes nothing for other plugins except the newly created discover-shared. I moved its implementation into the plugin (f53b567) and removed the package to keep the implementation more concise.

Copy link

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

LGTM - I've ignored anything specific to the fly-out code since I'll review that in elastic#180262

@tonyghiani
Copy link
Owner Author

Thank you all, I'll reopen this PR against main once the other one will be merged to keep this implementation tracked on its own commit with some PR report, I might ask you an approval to merge it.

MadameSheema and others added 21 commits April 24, 2024 10:48
Following up from elastic#180773

## Summary
 
In this PR we are introducing the `@skipServerlessMKI` label, with it, a
test will be excluded from the execution on any MKI environment but it
will be executed as part of the CI check if the `@serverless` tag is
present.
 
 With the new changes the serverless labels will work as follows:
 
* `@serverless`: The test is executed as part of the PR check process
and in the periodic pipeline (MKI environment).
 
* `@serverlessQA`: The test is executed as part of the kibana release
process in the QA environment (MKI environment).
 
* `@skipInServerless`: The test is skipped from being executed in CI as
part of the PR check and is skipped from being executed in any MKI
environment.
 
`@skipInServerlessMKI`: The test is skipped from being executed in any
MKI environment but it will continue being executed as part of the PR
process if the `@serverless` tag is present.
 
 **IMPORTANT:**
 
The skip labels have been created for those tests that use `@serverless`
or `@serverlessQA` labels. The absence of them (`@serverless` or
`@serverlessQA`) will exclude automatically the execution of the test in
the targeted environments.
 
I.E: A test without `@serverlessQA` will never be executed as part of
the Kibana release process. A test without `@serverless` will never be
executed as part of the PR CI check neither the periodic pipeline.
elastic#181261

Check if `routeState` is defined before accessing it's properties in
`useCallback` dependency list.

With changes:


https://github.com/elastic/kibana/assets/29123534/4561c5d6-e354-4e0b-ac8a-dd231a26d722

Cypress tests when no undefined check is performed (application throws):
![Screenshot 2024-04-23 at 14 46
37](https://github.com/elastic/kibana/assets/29123534/ed7817dc-f11c-4f5e-bdf3-c6ee9f4c8ea6)
When we send over a conversation to the LLM for completion, we include a
system message. System messages are a way for the consumer (in this
case, us as developers) to control the LLM's behavior.

This system message was previously constructed by using a concept called
`ContextDefinition` - originally this was a way to define a set of
functions and behavior for a specific context, e.g. core functionality,
APM-specific functionality, platform-specific functionality etc. However
we never actually did anything with this, and much of its intended
functionality is now captured with the screen context API.

In elastic#179736, we added user
instructions, which are ways for the user to control the Assistant's
behaviour, by appending to the system message we construct with the
registered context definitions.

With this PR, we are making several changes:

- Remove the concept of concept definitions entirely
- Replace it with `registerInstruction`, which allows the consumer to
register pieces of text that will be included in the system message.
- `registerInstruction` _also_ takes a callback. That callback receives
the available function names for that specific chat request. For
instance, when we reach the function call limit, the LLM will have no
functions to call. This allows consumers to cater their instructions to
this specific scenario, which somewhat limits the possibility of the LLM
calling a function that it is not allowed to - Claude is especially
prone to this (likely related to the fact we use simulated function
calling).

This leads to the following functional changes:
- A system message is now constructed by combining the registered
instructions (system-specific) with the knowledge base and request
instructions (user-specific)
- `GET /internal/observability_ai_assistant/functions` no longer returns
the contexts. Instead it returns the system message
- `GET /internal/observability_ai_assistant/chat/complete` now creates a
system message at the start, and overrides the system message from the
request.
- For each invocation of `chat`, it re-calculates the system message by
"materializing" the registered instructions with the available function
names for that chat invocation

Additionally, I've made some attempted improvements to simulated
function calling:
- simplified the system message
- more emphasis on generating valid JSON (e.g. I saw multiline
delimiters `"""` which are not supported)
- more emphasis on not providing any input if the function does not
accept any parameters. e.g. Claude was trying to provide entire search
requests or SPL-like query strings as input, which led to
hallucinations)

There are also some other changes, which I've commented on in the file
changes.

**Addendum: I have pushed some more changes, related to the evaluation
framework (and running it with Claude). Will comment inline in
[`9ebd207`
(elastic#181058)](https://github.com/elastic/kibana/pull/181058/commits/9ebd207acd47c33077627356c464958240c9d446).**
## Summary



https://github.com/elastic/kibana/assets/1410658/de571e71-88c1-4576-94ee-55763ba8af98

<img width="873" alt="Screenshot 2024-04-23 at 16 27 52"
src="https://github.com/elastic/kibana/assets/1410658/bbc4fb1c-6dd5-49f2-bd52-9d47c41b0be8">



### 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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] 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)
… Lens cell action (elastic#181151)

## Summary

Original issue and steps to reproduce:
elastic#181120


Before:



https://github.com/elastic/kibana/assets/6295984/10c7a2ce-d814-4750-8481-8f05b55384f8




After:



https://github.com/elastic/kibana/assets/6295984/00dfbcb6-244b-4f2b-8dd4-a1f7435385cf



### Checklist

Delete any items that are not applicable to this PR.


- [x] [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

---------

Co-authored-by: Sergi Massaneda <sergi.massaneda@gmail.com>
Hides unavailable connectors (e.g. due to license mismatches).
…#181449)

The service samples and transaction samples are estimated values, so I'm
adding a tooltip to make it clear for users.

<img width="883" alt="Screenshot 2024-04-23 at 15 51 44"
src="https://github.com/elastic/kibana/assets/55978943/b71284ba-9104-45e5-9546-303279cb11f3">
Closes elastic#170492

## Summary

The PR Implements the Dataset Quality Flyout summary KPIs: "Docs count",
"Size", "Services", "Hosts" and "Degraded docs".

|Stateful|Serverless|
|:---:|:---|
|<img alt="Screenshot 2024-03-26 at 19 14 34"
src="https://github.com/elastic/kibana/assets/2748376/d75de56f-0916-48a6-a101-fc3d8f084f4d">|<img
alt="Screenshot 2024-03-26 at 17 02 05"
src="https://github.com/elastic/kibana/assets/2748376/46d58946-2ed5-4c21-b53c-be3d43e7b857">|

"Show all" links for "Services" and "Hosts" metrics depend on some
development in APM and Infra plugins and therefore will be implemented
in a follow up issue.

Note that "Size" metric is excluded on Serverless as the endpoint uses
ES's `_stats` endpoint which is not available on Serverless (at the time
of creation of this PR). The code contains some conditions and cases to
tackle this, which should be considered as a temporary measure. All of
the code related to these changes should either be removed or modified
once there's a way to calculate the size of an index/dataStream on
Serverless (see [related
](elastic#178954)). The following
changes are made in particular:
 - Size UI component on the Flyout will be hidden on Serverless
- `dataset_quality/data_streams/{dataStream}/details` endpoint will
return `NaN` for size on Serverless
- Unit, integration and end-to-end tests on Serverless handle
"sizeBytes" property accordingly
## Summary

Move the `"size": 0` prop to the body in the network flow aggregation
queries
## Summary

Updates various connector configurations.

- Box, Notion, Slack, Teams, and Zoom are set to run native.
- Oracle, Outlook and Gmail are GA now. 
- GraphQL and OpenText Documentum added as Tech Preview connector
clients.
- Fixed a few tooltip issues.
- Fixed a Teams `service_type` in integrations page.

Note, waiting for icons for GraphQL and OpenText Documentum before
merging. Screenshots below will have broken images until they are added.

"Select Connector" with basic license:
<img width="1008" alt="Screenshot 2024-04-18 at 15 33 30"
src="https://github.com/elastic/kibana/assets/1410658/113237fb-94e0-465f-8134-f09fc871bc96">
<img width="1028" alt="Screenshot 2024-04-18 at 15 33 35"
src="https://github.com/elastic/kibana/assets/1410658/c59d63bb-fd27-45d3-9e05-093fdf5af6d6">

Integrations tiles:
<img width="1244" alt="Screenshot 2024-04-18 at 14 51 28"
src="https://github.com/elastic/kibana/assets/1410658/314c2c0e-722e-400d-a933-3b99190181b2">
<img width="577" alt="Screenshot 2024-04-18 at 14 51 34"
src="https://github.com/elastic/kibana/assets/1410658/56ef87b9-4ffc-4762-a37e-fa38a2a98c0c">

Native Upgrades:
<img width="342" alt="Screenshot 2024-04-18 at 14 50 12"
src="https://github.com/elastic/kibana/assets/1410658/324d006d-9f01-4761-be72-1346f88e47c0">
<img width="346" alt="Screenshot 2024-04-18 at 14 50 21"
src="https://github.com/elastic/kibana/assets/1410658/d5d67838-1459-4a2a-9480-4c6e2fd6035b">
<img width="352" alt="Screenshot 2024-04-18 at 14 50 32"
src="https://github.com/elastic/kibana/assets/1410658/4622d3bd-67b4-442b-9264-0d7ae889b8e7">
<img width="347" alt="Screenshot 2024-04-18 at 14 50 36"
src="https://github.com/elastic/kibana/assets/1410658/929ea8fa-fadf-46ea-b23b-eaaa193e169e">
<img width="378" alt="Screenshot 2024-04-18 at 14 50 40"
src="https://github.com/elastic/kibana/assets/1410658/44ae8fd6-5791-49c3-b4ef-7b7cb49f5038">


### Checklist

Delete any items that are not applicable to this PR.

- [x] 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

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
## Summary

Fix elastic#83409

Use a permanent cache (`public, max-age=365d, immutable`) for
translation files when in production (`dist`), similar to what we're
doing for static assets.

Translation files cache busting is a little tricky, because it doesn't
only depend on the version (enabling or disabling a custom plugin can
change the translations while not changing the build hash), so we're
using a custom hash generated from the content of the current
translation file (which was already used to generate the `etag` header
previously).

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Closes elastic/search-docs-team#100

- Update Introduction notebook to explain the what, how, and why of
notebooks, clarify that available as preview (not actually runnable in
our UI) currently, how to use in Colab and locally, link to Search Labs
- Update notebook preview snippets to concise descriptions
- Update link to search labs notebooks, update button text
- General rewordings

## Before


https://github.com/elastic/kibana/assets/32779855/a17d9b26-814b-4303-aac6-a8ef0a178ecf



## After



https://github.com/elastic/kibana/assets/32779855/c8cfd685-c89b-4726-b89d-babcc7fbc3cf
Fixes elastic#174205

## Summary

The failing test was rendering the opened modal and not doing anything
with it. I removed that block and the execution time locally went from
200+ ms to around 4.
## Summary

Fixes elastic#179814

Query string from the Unified Search query bar was applying to the
change point agg request, but wasn't in sync with the query service to
correctly render the chart preview. With the PR the query string is
synchronized and metric charts are rendered correctly.


### Checklist

- [ ] [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
## Summary

Async registration is bad, especially when promise rejections are not
caught.

The PR adapts the routes registration to be synchronous
…tic#181431)

## Summary

We were using the wrong colour variables in a couple of places.
These changes only impact the dark mode UI.


Fixed pages:

![Screenshot 2024-04-23 at 14 55
20](https://github.com/elastic/kibana/assets/1490444/4b330c43-559d-4542-ab6b-46e98b69ce19)

![Screenshot 2024-04-23 at 14 36
12](https://github.com/elastic/kibana/assets/1490444/0b47b893-e9f2-4a54-b7c6-d055f2bb91a6)

.
…raded docs chart (elastic#181509)

## Summary

The PR adds the `breakdownField` param in `LogsExplorerNavigationParams`
so that when "Explorer data in Logs Explorer" is clicked on Degraded
Docs chart on Dataset Quality flyout while the chart has a breakdown
field selected, the field is passed over to Logs Explorer.



https://github.com/elastic/kibana/assets/2748376/b380ac85-e40e-451b-983f-41c68f87ed7b
## Summary

Set a system user name when the request is fake. This is helpful to show
user information to the case created by the case action.

## Testing

1. Create a rule with a case action
2. In the created case verify that the `elastic/kibana` user is shown
and not the `Unknown` user

<img width="2297" alt="Screenshot 2024-04-22 at 10 37 46 PM"
src="https://github.com/elastic/kibana/assets/7871006/abfcec4c-f2a4-4663-84e0-1816ada69167">


### Checklist

Delete any items that are not applicable to this PR.

- [x] [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

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary

Closes elastic#181530

- Sets percentage width for all columns
- Sets responsive breakpoint 
- Makes Deployment stats table responsive as well 

![Apr-24-2024
12-16-48](https://github.com/elastic/kibana/assets/5236598/2a14ffb9-de15-45e9-b8bc-276e10080864)


### Checklist

- [x] 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))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
pgayvallet and others added 15 commits April 29, 2024 00:34
…ty` plugin (elastic#181538)

## Summary

Part of elastic#174578

Adapt Core's `userSettings` service to leverage Core's `userProfile`
service instead of the `security` plugin

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## Summary
the job was intended to run 1x day - but due to default values, it was
getting triggered on every commit & elastic/kibana branches
closes elastic#181017

## 📝  Summary

This PR adds a quality filter above the dataset quality table to allow
filtering the table using qualities. The selected filters are synced
with the URL as well.

The Degraded docs column is also split into 2 different columns
`Degraded Docs (%)` & `Dataset Quality`

## 🎥 Demo


https://github.com/elastic/kibana/assets/11225826/e72993d0-483d-4a96-8266-ab0c3d3e5bab
…tic#181811)

## Summary

This PR disables the default behaviour of tracking total hits. This
should make the request faster.

Before:
<img width="759" alt="Screenshot 2024-04-26 at 10 52 41"
src="https://github.com/elastic/kibana/assets/1415710/250e3538-0607-460e-b5b1-3d81a29a391c">

After:
<img width="768" alt="Screenshot 2024-04-26 at 10 58 50"
src="https://github.com/elastic/kibana/assets/1415710/76ce7bdf-918b-48ba-ad5a-c1459d6931b2">
## Summary

Based on the introduction of new response schemas for OAS generation we
are going to start the long tail of introducing missing response (`joi`)
schemas. We have roughly 520 known public APIs, most of which do not
have response schemas defined. We expected a fairly large increase in
`@kbn/config-schema` definitions in the coming weeks/months. Regardless
of actual outcome and given how slow schema instantiation is, this
presents a slight concern for startup time.

## Proposed changes

Give consumers guidance and a way to pass in validation lazily. Under
the hood we make sure that the lazy schemas only get called once.

```ts

/**
 * A validation schema factory.
 *
 * @note Used to lazily create schemas that are otherwise not needed
 * @note Assume this function will only be called once
 *
 * @return A @kbn/config-schema schema
 * @public
 */
export type LazyValidator = () => Type<unknown>;

/** @public */
export interface VersionedRouteCustomResponseBodyValidation {
  /** A custom validation function */
  custom: RouteValidationFunction<unknown>;
}

/** @public */
export type VersionedResponseBodyValidation =
  | LazyValidator
  | VersionedRouteCustomResponseBodyValidation;

/**
 * Map of response status codes to response schemas
 *
 * @note Instantiating response schemas is expensive, especially when it is
 *       not needed in most cases. See example below to ensure this is lazily
 *       provided.
 *
 * @note The {@link TypeOf} type utility from @kbn/config-schema can extract
 *       types from lazily created schemas
 *
 * @example
 * ```ts
 * // Avoid this:
 * const badResponseSchema = schema.object({ foo: foo.string() });
 * // Do this:
 * const goodResponseSchema = () => schema.object({ foo: foo.string() });
 *
 * type ResponseType = TypeOf<typeof goodResponseSchema>;
 * ...
 * .addVersion(
 *  { ... validation: { response: { 200: { body: goodResponseSchema } } } },
 *  handlerFn
 * )
 * ...
 * ```
 * @public
 */
export interface VersionedRouteResponseValidation {
  [statusCode: number]: {
    body: VersionedResponseBodyValidation;
  };
  unsafe?: { body?: boolean };
}
```

## Notes

* Expected (worst case) in low resource environments is an additional
1.5 seconds to start up time and additional ~70MB to memory pressure
which is not a great trade-off for functionality that is only used when
OAS generation is on.

Related elastic#181277
…s from shared bundle (elastic#181543)

## Summary

This PR aims to reduce the amount of dependencies used in the
visualizations plugins.

Dependencies removed:
* `jquery` usage in `graph`
* `jquery` usage in `Vega` plugin.

As a bonus point it makes the `@kbn/flot-charts` dependency a Canvas and
Monitoring only (the only 2 consumers) rather than load it for all
Kibana plugins.

### Checklist

- [x] [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

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
… to expand/collapse sections need unique accessible labels (elastic#180907)

Closes: elastic/security-team#8929

## Description
The Getting Started page has several accordions with arrow buttons to
expand and collapse the sections. These buttons have generic "Expand"
accessible labels that do not provide enough information to screen
readers and voice activation technologies. We should update the
accessible labels to include the section name.

### Steps to recreate

1. Open the [QA environment](https://console.qa.cld.elstc.co/) and
create a new Security serverless project if none exist
2. Once your Security project is created, open it and turn on a screen
reader of your choice
3. Load the Getting Started page (should be by default, but just in
case)
4. Tab through the accordion sections and verify each one says "Expand"
when the arrow button takes focus

### What was done?: 
1. The `aria-expanded` attribute was added to the toggle-button.
2. The `aria-label` was updated to display the title part.
3. The `COLLAPSE_STEP_BUTTON_LABEL` translation was removed.

### Screen 
<img width="1565" alt="image"
src="https://github.com/elastic/kibana/assets/20072247/497077b8-c272-4985-8a85-653e1f29d776">
…lastic#180262)

## 📓 Summary

Closes elastic#180024 

This work aims to move the "Logs Overview" doc view created from the
logs-explorer app into the unified-doc-viewer plugin, creating and
registering this as a preset along the table and source doc views.

To keep control of whether this doc view should be displayed or not, an
`enabled` configuration flag is supported for every doc view and will be
used to determine whether a doc view should load or not in the view.
This `enabled` flag can be programmatically enabled/disabled by
`docView.id` using the 2 new methods added to the `DocViewsRegistry`,
the `enableById` and `disableById` ones.
The customization extension point does not register the content of the
tab, but is limited to enable/disable a preset overview tab.

To allow this change, some shared utilities between the flyout logic and
the smart fields feature have been copied into the `@kbn/discover-utils`
package. The utils will still live in the logs_explorer plugin and are
used by the smart fields feature until this is migrated too into
Discover.

## 💡 Reviewer hints

Although it seems a large PR, most of the changes are on moved files
from logs-explorer into unified-doc-viewer, which is logic already
reviewed. With these changes, there will be a follow-up PR to optimize
the shared parts with the other doc views.

## 🚶 Next steps

To keep the scope of this PR the smallest possible, here are some
changes I'll work out in upcoming PRs built on top of this one.

- [x] Implement a discover registry to enable registering external
features, such as the ObservabilityAIAssistant.
- [x] Integrate ObsAIAssistant with this work through the new discover
features registry.
- [x] Refactor the doc views to share duplicated logic.
- [x] Port the existing e2e tests for the logs overview tab into the
unified-doc-viewer plugin.

#1

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
)

This is a fix for
elastic/sdh-security-team#947

There is no easy way to reproduce this but you can try and change the
line in question so that the faulty username is used instead of the one
passed in.
…astic#181941)

The data-test-subj of the first question was only set on the radio
button which means it's not recorded when the text is clicked.

This moves it up to the wrapping element so it's always captured
correctly.
…be able to take keyboard focus (elastic#180678)

Closes: elastic/security-team#8898

## Description

The Security > Get Started view for Serverless projects has a number of
accordions that do not remove focusable content from the DOM. This can
lead to a situation where keyboard users' focus is lost inside a closed
accordion. This can create bad UX as the number of focusable elements
inside an accordion increase. Screenshot attached below.

### Steps to recreate

1. Open [Cloud UI](https://console.qa.cld.elstc.co/) and create a
Serverless Security project if none exist
2. When the project is created, click the **Continue** button
3. Navigate to the Security project **Get started** view. This link is
in the lower left of the navigation pane.
4. Tab through the page until you see focus on the downward arrow on an
accordion. Tab again and notice the focus disappears.
5. Verify focus is on a hidden element by opening the Console and typing
`document.activeElement` into the prompt. This will show focus is on an
element inside the accordion.

### What was done?: 

1.
[visible](https://developer.mozilla.org/en-US/docs/Web/CSS/visibility#visible)
style was added. Using a visibility value of hidden/collapse on an
element will remove it from the [accessibility
tree](https://developer.mozilla.org/en-US/docs/Learn/Accessibility/What_is_accessibility#accessibility_apis).

### Screen: 


https://github.com/elastic/kibana/assets/20072247/acf62fe0-7a12-4fdc-9961-2ecf17c20e0b
…astic#181837)

## Summary



https://github.com/elastic/kibana/assets/1410658/76067c45-7f69-4b3e-939c-d6746a2963a6




### Checklist

Delete any items that are not applicable to this PR.

- [ ] [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
- [x] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] 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))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
…stic#181425)

closes [elastic#178714](elastic#178714)

## Summary

This PR changes where link to legacy uptime is displayed to
conditionally show it according to whether the plugin is enabled or not.

<img width="962" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/e2e659ae-bc2e-49a6-a34b-9434f04b4617">

<img width="962" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/4aa487f5-4a61-4387-a9ed-12c9ed624410">


### How to test

The issue and the fix are the same in serverless and stateful

#### APM 
- Run
```
node scripts/synthtrace simple_trace.ts --from=now-15m --to=now --clean --target=http://elastic_serverless:changeme@localhost:9200 --kibana=http://elastic_serverless:changeme@0.0.0.0:5601
```
- Navigate to APM > Traces, click on "Investigate" 
- if `xpack.legacy_uptime.enabled: false` in kibana.yml - "Status"
**should** appear
- if `xpack.legacy_uptime.enabled: false` in kibana.yml - "Status"
**should not** appear
- if `xpack.legacy_uptime.enabled: false` and `xpack.uptime.enabled:
true` in kibana.yml - "Status" **should not** appear
- if `observability:enableLegacyUptimeApp` is switched on in Advanced
settings - "Status" **should** appear

#### Infra 
- Start a local kibana instance pointing to an oblt cluster
- Navigate to Infrastructure, select `Kubernetes Pod` (other asset
types, except hosts, share the same code) in `Show` dropdown
- if `xpack.legacy_uptime.enabled: false` in kibana.yml - the link
should be **enabled**
- if `xpack.legacy_uptime.enabled: false` in kibana.yml - the link to
uptime should be **disabled**
- if `xpack.legacy_uptime.enabled: false` and `xpack.uptime.enabled:
true` in kibana.yml - the link to uptime should be **disabled**
- if `observability:enableLegacyUptimeApp` is switched on in Advanced
settings - the link to uptime should be **enabled**

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@tonyghiani
Copy link
Owner Author

Closing in favour of elastic#181952

@tonyghiani tonyghiani closed this Apr 30, 2024
tonyghiani added a commit to elastic/kibana that referenced this pull request May 3, 2024
## 📓 Summary

Closes #181528 
Closes #181976 

**N.B.** This work was initially reviewed on a separate PR at
tonyghiani#1.

This implementation aims to have a stateful layer that allows the
management of dependencies between Discover and other plugins, reducing
the need for a direct dependency.

Although the initial thought was to have a plugin to register features
for Discover, there might be other cases in future where we need to
prevent cyclic dependencies.
With this in mind, I created the plugin as a more generic solution to
hold stateful logic as a communication layer for Discover <-> Plugins.

## Discover Shared

Based on some recurring naming in the Kibana codebase, `discover_shared`
felt like the right place for owning these dependencies and exposing
Discover functionalities to the external world.
It is initially pretty simple and only exposes a registry for the
Discover features, but might be a good place to implement other upcoming
concepts related to Discover.

Also, this plugin should ideally never depend on other solution plugins
and keep its dependencies to a bare minimum of packages and core/data
services.

```mermaid
flowchart TD

A(Discover) -- Get --> E[DiscoverShared]
B(Logs Explorer) -- Set --> E[DiscoverShared]
C(Security) -- Set --> E[DiscoverShared]
D(Any app) -- Set --> E[DiscoverShared]
```

## DiscoverFeaturesService

This service initializes and exposes a strictly typed registry to allow
consumer apps to register additional features and Discover and retrieve
them.

The **README** file explains a real use case of when we'd need to use it
and how to do that step-by-step.

Although it introduces a more nested folder structure, I decided to
implement the service as a client-service and expose it through the
plugin lifecycle methods to provide the necessary flexibility we might
need:
- We don't know yet if any of the features we register will be done on
the setup/start steps, so having the registry available in both places
opens the door to any case we face.
- The service is client-only on purpose. My opinion is that if we ever
need to register features such as server services or anything else, it
should be scoped to a similar service dedicated for the server lifecycle
and its environment.
It should never be possible to register the ObsAIAssistant
presentational component from the server, as it should not be permitted
to register a server service in the client registry.
A server DiscoverFeaturesService is not required yet for any feature, so
I left it out to avoid overcomplicating the implementation.

## FeaturesRegistry

To have a strictly typed utility that suggests the available features on
a registry and adheres to a base contract, the registry exposed on the
DiscoverFeaturesService is an instance of the `FeaturesRegistry` class,
which implements the registration/retrieval logic such that:
- If a feature is already registered, is not possible to override it and
an error is thrown to notify the user of the collision.
- In case we need to react to registry changes, is possible to subscribe
to the registry or obtain it as an observable for more complex
scenarios.

The FeaturesRegistry already takes care of the required logic for the
registry, so that `DiscoverFeaturesService`is left with the
responsibility of instantiating/exposing an instance and provide the set
of allowed features.

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…lastic#180262)

## 📓 Summary

Closes elastic#180024 

This work aims to move the "Logs Overview" doc view created from the
logs-explorer app into the unified-doc-viewer plugin, creating and
registering this as a preset along the table and source doc views.

To keep control of whether this doc view should be displayed or not, an
`enabled` configuration flag is supported for every doc view and will be
used to determine whether a doc view should load or not in the view.
This `enabled` flag can be programmatically enabled/disabled by
`docView.id` using the 2 new methods added to the `DocViewsRegistry`,
the `enableById` and `disableById` ones.
The customization extension point does not register the content of the
tab, but is limited to enable/disable a preset overview tab.

To allow this change, some shared utilities between the flyout logic and
the smart fields feature have been copied into the `@kbn/discover-utils`
package. The utils will still live in the logs_explorer plugin and are
used by the smart fields feature until this is migrated too into
Discover.

## 💡 Reviewer hints

Although it seems a large PR, most of the changes are on moved files
from logs-explorer into unified-doc-viewer, which is logic already
reviewed. With these changes, there will be a follow-up PR to optimize
the shared parts with the other doc views.

## 🚶 Next steps

To keep the scope of this PR the smallest possible, here are some
changes I'll work out in upcoming PRs built on top of this one.

- [x] Implement a discover registry to enable registering external
features, such as the ObservabilityAIAssistant.
- [x] Integrate ObsAIAssistant with this work through the new discover
features registry.
- [x] Refactor the doc views to share duplicated logic.
- [x] Port the existing e2e tests for the logs overview tab into the
unified-doc-viewer plugin.

tonyghiani#1

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
yuliacech pushed a commit to yuliacech/kibana that referenced this pull request May 3, 2024
…ic#181952)

## 📓 Summary

Closes elastic#181528 
Closes elastic#181976 

**N.B.** This work was initially reviewed on a separate PR at
tonyghiani#1.

This implementation aims to have a stateful layer that allows the
management of dependencies between Discover and other plugins, reducing
the need for a direct dependency.

Although the initial thought was to have a plugin to register features
for Discover, there might be other cases in future where we need to
prevent cyclic dependencies.
With this in mind, I created the plugin as a more generic solution to
hold stateful logic as a communication layer for Discover <-> Plugins.

## Discover Shared

Based on some recurring naming in the Kibana codebase, `discover_shared`
felt like the right place for owning these dependencies and exposing
Discover functionalities to the external world.
It is initially pretty simple and only exposes a registry for the
Discover features, but might be a good place to implement other upcoming
concepts related to Discover.

Also, this plugin should ideally never depend on other solution plugins
and keep its dependencies to a bare minimum of packages and core/data
services.

```mermaid
flowchart TD

A(Discover) -- Get --> E[DiscoverShared]
B(Logs Explorer) -- Set --> E[DiscoverShared]
C(Security) -- Set --> E[DiscoverShared]
D(Any app) -- Set --> E[DiscoverShared]
```

## DiscoverFeaturesService

This service initializes and exposes a strictly typed registry to allow
consumer apps to register additional features and Discover and retrieve
them.

The **README** file explains a real use case of when we'd need to use it
and how to do that step-by-step.

Although it introduces a more nested folder structure, I decided to
implement the service as a client-service and expose it through the
plugin lifecycle methods to provide the necessary flexibility we might
need:
- We don't know yet if any of the features we register will be done on
the setup/start steps, so having the registry available in both places
opens the door to any case we face.
- The service is client-only on purpose. My opinion is that if we ever
need to register features such as server services or anything else, it
should be scoped to a similar service dedicated for the server lifecycle
and its environment.
It should never be possible to register the ObsAIAssistant
presentational component from the server, as it should not be permitted
to register a server service in the client registry.
A server DiscoverFeaturesService is not required yet for any feature, so
I left it out to avoid overcomplicating the implementation.

## FeaturesRegistry

To have a strictly typed utility that suggests the available features on
a registry and adheres to a base contract, the registry exposed on the
DiscoverFeaturesService is an instance of the `FeaturesRegistry` class,
which implements the registration/retrieval logic such that:
- If a feature is already registered, is not possible to override it and
an error is thrown to notify the user of the collision.
- In case we need to react to registry changes, is possible to subscribe
to the registry or obtain it as an observable for more complex
scenarios.

The FeaturesRegistry already takes care of the required logic for the
registry, so that `DiscoverFeaturesService`is left with the
responsibility of instantiating/exposing an instance and provide the set
of allowed features.

---------

Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Davis McPhee <davismcphee@hotmail.com>
tonyghiani pushed a commit that referenced this pull request May 20, 2024
## Summary
Set `security.session.cleanupInterval` to 5h for session concurrency
test.

### **Prerequisites**

- Task for session cleanup with [default schedule set to
1h](https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/server/config.ts#L222).
- Task polling interval is set to
[3000ms](https://github.com/elastic/kibana/blob/main/x-pack/plugins/task_manager/server/config.ts#L13).
- We override `scheduledAt` once we make a request in
[runCleanupTaskSoon](https://github.com/elastic/kibana/blob/main/x-pack/test/security_api_integration/tests/session_concurrent_limit/cleanup.ts#L145).

### **Hypothesis**

Taking into consideration that:

- `session_cleanup` task is not the only one scheduled during test run.
- There is sort of an exponential backoff implemented for task polling
if there are too many retries.
- Clock jitter.

I had a hypothesis that if our whole test run exceeds 1h or polling
interval gets adjusted because of retries we might end up executing the
scheduled cleanup before we trigger `runCleanupTaskSoon` (this is there
we drop 1 session already).

### **FTR runs (x55 each)**

- `cleanupInterval` set to 5h:
[#1](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5986)
:green_circle:,
[elastic#2](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5987)
:green_circle:
- `cleanupInterval` set to default 1h:
[#1](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5983)
:green_circle:,
[elastic#2](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5982)
:red_circle: (has 2 failures out of 55)


### Checklist

- [x] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

__Fixes: https://github.com/elastic/kibana/issues/149091__
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet