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

[Infra] Deprecate fields returned by the /api/metrics/source endpoint, in favor of ad hoc data views #181954

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Apr 29, 2024

closes #180689

Summary

This PR lays the necessary foundation to deprecate the indices fields list attribute retrieved and returned by the api/metrics/source endpoint and affects all infra pages.

Now instead of the Metrics UI and Inventory UI relying on that attribute to build the list of suggestions in their search bars, an adhoc data view will be created from the metricsAlias returned by the same endpoint. The data view object contains everything that's needed with the benefit of allowing us to make async requests and cache, provided by Data Views Service.

This will allow us to start moving away from api/metrics/source to retrieve information critical to the loading of the pages.

It's a big PR, and the changes affect mostly the search bars, field name selectors, and charts across infra. Besides, it also contains

  • Storybook fix.
  • Some components were turned into functional components
  • <MetricsPageTemplate /> now encapsulates error messages and is only used in Infrastructure pages
  • Improvement of the Infrastructure routing code.
  • Data View provider common to all infra pages
  • SourceProvider refactor.
image image image image image image image

How to test

  • Start a local kibana instance pointing to an Oblt cluster
  • Navigate to Inventory, and all its pages, check if charts, search bars and field name autocomplete fields are working

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@crespocarlos crespocarlos force-pushed the 180689-deprecate-fields-from-source-endpoint branch from 50bb70e to 63ea9bb Compare April 29, 2024 14:54
@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos crespocarlos force-pushed the 180689-deprecate-fields-from-source-endpoint branch from 63ea9bb to 33899c1 Compare April 29, 2024 16:50
@crespocarlos crespocarlos force-pushed the 180689-deprecate-fields-from-source-endpoint branch 4 times, most recently from e56ab19 to a511639 Compare April 30, 2024 11:02
@crespocarlos
Copy link
Contributor Author

/ci

@@ -8,83 +8,76 @@
import { EuiButton, EuiComboBox, EuiForm, EuiFormRow } from '@elastic/eui';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a functional component

@@ -15,193 +15,176 @@ import {
import { i18n } from '@kbn/i18n';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it a functional component

@crespocarlos crespocarlos force-pushed the 180689-deprecate-fields-from-source-endpoint branch from a511639 to a7eb928 Compare April 30, 2024 12:14
@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos crespocarlos added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v8.15.0 labels Apr 30, 2024
return <SourceLoadingPage />;
}

return (
<MetricsPageTemplate
<PageTemplate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want all the validations that happen in <MetricsPageTemplate /> in the Settings page

return (
<MetricsPageTemplate
<PageTemplate
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't want all the validations that happen in <MetricsPageTemplate /> in the Settings page

@crespocarlos crespocarlos marked this pull request as ready for review April 30, 2024 17:05
@crespocarlos crespocarlos requested review from a team as code owners April 30, 2024 17:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@crespocarlos crespocarlos changed the title Stop using fields returned by the source endpoint, in favor of ad hoc… [Infra] Deprecate fields returned by the /api/metrics/source endpoint, in favor of ad hoc data views Apr 30, 2024
@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

services: { dataViews },
} = useKibanaContextForPlugin();

const { source } = useSourceContext();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're planning to remove the index pattern from here we could replace this with something that fetches for the index pattern in the metrics data access plugin. Is that what you're thinking to do in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

exactly, this will be removed once we have a new endpoint to return the index pattern from metrics data access plugin

@@ -120,15 +120,12 @@ export const defaultExpression = {

export const Expressions: React.FC<Props> = (props) => {
const { setRuleParams, ruleParams, errors, metadata } = props;
const { source, createDerivedIndexPattern } = useSourceContext();
const { source } = useSourceContext();
Copy link
Contributor

@neptunian neptunian May 2, 2024

Choose a reason for hiding this comment

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

how is this being used here? seems like we may not need it with the inclusion of metricsView.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. we need the sourceId. Should we create a ticket to stop using this attribute? I don't really see the point of having it in the current solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, missed that

Copy link
Member

Choose a reason for hiding this comment

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

@elastic/obs-ux-management-team we might want to log the ticket Carlos mentions here and get rid of the source entirely if it's no longer used/needed, in favor of data views

} = useKibanaContextForPlugin();

const { value: filters = [] } = useAsync(async () => {
const resolvedDataView = await resolveDataView({
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the reasoning behind using this instead of useMetricsDataViewContext().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's because we need to retrieve the logs data view too for the logs rate chart.

This chart component is data view agnostic and it depends on the index pattern information that the chart objects hold. This is only needed to build filters in a way that they look nicer when the user opens the chart in Lens. If it wasn't for that, this whole section could've been removed.

@neptunian
Copy link
Contributor

Thanks for tackling this!

Copy link
Contributor

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

Looks good! I added some nits/questions.

<DropdownButton
label={i18n.translate('xpack.infra.waffle.groupByLabel', { defaultMessage: 'Group by' })}
onClick={togglePopover}
data-test-subj={'waffleGroupByDropdown'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit

Suggested change
data-test-subj={'waffleGroupByDropdown'}
data-test-subj="waffleGroupByDropdown"

name?: string;
}

export async function resolveDataView({
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need async here?

@@ -12,7 +12,7 @@ import {
type Filter,
isCombinedFilter,
} from '@kbn/es-query';
import type { DataView } from '@kbn/data-views-plugin/common';
import { DataView } from '@kbn/data-views-plugin/common';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this change needed?

@@ -17,8 +17,8 @@ import { MetricK8sModuleProvider } from '../../../containers/ml/modules/metrics_
import { useActiveKibanaSpace } from '../../../hooks/use_kibana_space';

export const AnomalyDetectionFlyout = ({
hideJobType,
hideSelectGroup,
hideJobType = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need those default values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's better than letting them be undefined in case it's not informed

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked because we have them as optional also in other places where we check the value but it's Ok to have them set to false by default.

timestamps: MetricsExplorerTimestamp;
enabled?: boolean;
}) {
// Your code logic here
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this comment?

Suggested change
// Your code logic here

@kdelemme kdelemme self-assigned this May 6, 2024
@kdelemme kdelemme self-requested a review May 6, 2024 14:39
@kdelemme kdelemme removed their assignment May 6, 2024
Copy link
Contributor

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM 💯 Thanks for the fixes!

@kdelemme
Copy link
Contributor

kdelemme commented May 6, 2024

@crespocarlos I'm having difficulties to see data in the Inventory pages using the edge-oblt and edge-oblt-lite. Which one should I use?

@crespocarlos
Copy link
Contributor Author

@crespocarlos I'm having difficulties to see data in the Inventory pages using the edge-oblt and edge-oblt-lite. Which one should I use?

Hi @kdelemme . It should work with both. If pointing your local instance to an oblt cluster, check if you've configured the index pattern in the settings page.

image

value: remote_cluster:metrics-*,remote_cluster:metricbeat-*

@kdelemme
Copy link
Contributor

kdelemme commented May 6, 2024

Ah thanks ! Forgot about this..

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

Code change LGTM

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented May 27, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1561 1560 -1

Async chunks

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

id before after diff
infra 1.5MB 1.5MB -1.1KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count - 5412 +5412
total size - 8.8MB +8.8MB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
infra 105.6KB 105.7KB +122.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@crespocarlos crespocarlos merged commit 982dfa3 into elastic:main May 27, 2024
22 checks passed
@crespocarlos crespocarlos deleted the 180689-deprecate-fields-from-source-endpoint branch May 27, 2024 10:14
rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024
…nt, in favor of ad hoc data views (elastic#181954)

closes [elastic#180689](elastic#180689)

## Summary

This PR lays the necessary foundation to deprecate the indices [fields
list attribute retrieved and
returned](https://github.com/elastic/kibana/blob/main/x-pack/plugins/observability_solution/infra/server/routes/metrics_sources/index.ts#L38)
by the `api/metrics/source` endpoint and affects all infra pages.

Now instead of the Metrics UI and Inventory UI relying on that attribute
to build the list of suggestions in their search bars, an adhoc data
view will be created from the `metricsAlias` returned by the same
endpoint. The data view object contains everything that's needed with
the benefit of allowing us to make async requests and cache, provided by
Data Views Service.

This will allow us to start moving away from `api/metrics/source` to
retrieve information critical to the loading of the pages.

It's a big PR, and the changes affect mostly the search bars, field name
selectors, and charts across infra. Besides, it also contains
- Storybook fix.
- Some components were turned into functional components
- `<MetricsPageTemplate />` now encapsulates error messages and is only
used in Infrastructure pages
- Improvement of the Infrastructure routing code.
- Data View provider common to all infra pages
- `SourceProvider` refactor.

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/ceb781ed-2464-4f67-93e4-7013786830d4">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/169f6f6b-03da-48e3-86b8-449bad1b2035">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/3e5b8394-ead2-40bb-ad00-609b29b179e5">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/34639ebf-af75-46e3-840b-8aa89951ae6e">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/b6ff3b96-219b-4daa-bc33-f9fd36912a5d">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/437cb7b8-62a6-4e1a-b94e-41055ab71bc1">

<img width="921" alt="image"
src="https://github.com/elastic/kibana/assets/2767137/72f04712-7ee1-44fc-990e-83f0fbff7115">

## How to test

- Start a local kibana instance pointing to an Oblt cluster
- Navigate to Inventory, and all its pages, check if charts, search bars
and field name autocomplete fields are working

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team technical debt Improvement of the software architecture and operational architecture v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra] Create a data view client
9 participants