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

[SLO Form] Refactor to use kibana data view component #173513

Merged
merged 16 commits into from
Jan 3, 2024

Conversation

shahzad31
Copy link
Contributor

@shahzad31 shahzad31 commented Dec 18, 2023

Summary

Fixes #171687

Refactor to use kibana data view component !!

image

user can also save a new data or use ad-hoc data view

image

This also avoid so many unnecessary requests being loaded, notice the difference almost avoid 15 extra requests

After

image

Before

image

@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.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@shahzad31
Copy link
Contributor Author

/ci

@shahzad31 shahzad31 added the release_note:skip Skip the PR/issue when compiling release notes label Dec 18, 2023
@shahzad31 shahzad31 marked this pull request as ready for review December 18, 2023 15:54
@shahzad31 shahzad31 requested a review from a team as a code owner December 18, 2023 15:54
@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Dec 18, 2023
@elasticmachine
Copy link
Contributor

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

@simianhacker simianhacker self-requested a review December 18, 2023 22:09
@simianhacker
Copy link
Member

simianhacker commented Dec 18, 2023

It looks like there's an issue when you go to clone an SLO which was created using an AdHoc DataView, the page hangs. Here of the steps I used:

  1. Index some fake_host data using the High Cardinality Indexer
  2. Create a "Timeslice Metric"
  3. Create an adhoc DataView (name: Fake Hosts, indexPattern: high-cardinality-data-fake_hosts.fake_hosts-*, timestamp: @timestamp)
  4. Save the SLO
  5. Clone the SLO (this is where the browser hangs)

Also since we are using DataViews, do you think we could set the Timestamp field to default to what's set for the DataView? Then they can change it if they want a different field.

Comment on lines +85 to +89
dataViewsService.get(newId).then((dataView) => {
if (dataView.timeFieldName) {
setValue('indicator.params.timestampField', dataView.timeFieldName);
}
});
Copy link
Member

Choose a reason for hiding this comment

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

❤️

@simianhacker
Copy link
Member

Good News: Not sure why it the clone action was hanging... I did a yarn kbn reset and restarted everything and now it's working.

Bad News: Looks like there is a blank temporary DataView selected when you click on "Create SLO" from the overview page:

image

Then when I create an SLO and edit it, I get this:

image

The blank temporary DataView is in the list along with a temporary DataView with the configured index pattern. The selected DataView is the DataView I use to create the SLO. I would expect to just see the "Admin Console" DataView selected.

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

Thanks for removing the "empty" temporary DataView. For the "extra" data view, I added a comment with an example that would remove that as well. So the only time we'd show an "adhoc" DataView would be if there wasn't a suitable DataView to use.

Comment on lines 31 to 51
useEffect(() => {
const missingAdHocDataView = adHocDataViews.find(
(dataView) => dataView.getIndexPattern() === currentIndexPattern
);

const placeholder = i18n.translate('xpack.observability.slo.sloEdit.indexSelection.placeholder', {
defaultMessage: 'Select an index pattern',
});
if (!missingAdHocDataView && currentIndexPattern) {
async function loadMissingDataView() {
const dataView = await dataViewsService.create(
{
title: currentIndexPattern,
allowNoIndex: true,
},
true
);
if (dataView.getIndexPattern() === currentIndexPattern) {
setAdHocDataViews((prev) => [...prev, dataView]);
}
}
loadMissingDataView();
}
}, [adHocDataViews, currentIndexPattern, dataViewsService]);
Copy link
Member

Choose a reason for hiding this comment

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

If you changed this useEffect this:

  useEffect(() => {
    if (!isDataViewsLoading) {
      const missingAdHocDataView =
        dataViews.find((dataView) => dataView.title === currentIndexPattern) ||
        adHocDataViews.find((dataView) => dataView.getIndexPattern() === currentIndexPattern);

      if (!missingAdHocDataView && currentIndexPattern) {
        async function loadMissingDataView() {
          const dataView = await dataViewsService.create(
            {
              title: currentIndexPattern,
              allowNoIndex: true,
            },
            true
          );
          if (dataView) {
            setAdHocDataViews((prev) => [...prev, dataView]);
          }
        }
        loadMissingDataView();
      }
    }
  }, [adHocDataViews, currentIndexPattern, dataViewsService, isDataViewsLoading, dataViews]);

Then it wouldn't list the index pattern as an adhoc index pattern when a suitable DataView exists.

Copy link
Member

@simianhacker simianhacker left a comment

Choose a reason for hiding this comment

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

LGTM

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jan 2, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 574 573 -1

Async chunks

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

id before after diff
observability 588.3KB 587.6KB -727.0B

History

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

@shahzad31 shahzad31 merged commit d56e221 into elastic:main Jan 3, 2024
22 checks passed
@shahzad31 shahzad31 deleted the data-view branch January 3, 2024 08:40
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 3, 2024
jloleysens added a commit that referenced this pull request Jan 4, 2024
* main: (4129 commits)
  [Logs Explorer] Change the default link for "Discover" in the serverless nav (#173420)
  [Fleet] fix unhandled error in agent details when components are missing (#174152)
  [Obs UX] Unskip transaction duration alerts test (#174069)
  [Fleet] Fix keep policies up to date after package install (#174093)
  [Profiling] Stack traces embeddable (#173905)
  [main] Sync bundled packages with Package Storage (#174115)
  [SLO Form] Refactor to use kibana data view component (#173513)
  [Obs UX] Unskip APM Service Inventory Journey (#173510)
  [Obs UX] Unskip preview_chart_error_count test (#173624)
  [api-docs] 2024-01-03 Daily api_docs build (#174142)
  Update babel runtime helpers (#171745)
  Handle content stream errors in report pre-deletion (#173792)
  [Cloud Posture] [Quick wins] Enable edit DataView on the Misconfigurations Findings Table (#173870)
  [ftr] abort retry on invalid webdriver session (#174092)
  Upgrade openai to 4.24.1 (#173934)
  chore(NA): bump node into v20 (#173461)
  [Security Solution][Endpoint] Fix index name pattern in SentinelOne dev. script (#174105)
  fix versions.json
  [Obs AI Assistant] Add guardrails (#174060)
  [ML] Transforms: Refactor validators and add unit tests. (#173736)
  ...
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 release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SLO] Fix index selector when entered index is from ccs
6 participants