Skip to content

[Logs] Use central log sources setting in Logs Explorer as the default data source#190438

Merged
Kerry350 merged 6 commits intoelastic:mainfrom
Kerry350:169-use-central-log-sources-setting-in-logs-explorer
Aug 22, 2024
Merged

[Logs] Use central log sources setting in Logs Explorer as the default data source#190438
Kerry350 merged 6 commits intoelastic:mainfrom
Kerry350:169-use-central-log-sources-setting-in-logs-explorer

Conversation

@Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Aug 13, 2024

Summary

Implements https://github.com/elastic/logs-dev/issues/169.

This uses the new central log sources setting as the default data source in the Logs Explorer. The AllSelection is now amended to use a set of indices, this AllSelection can be defined by the consumer (or falls back to a default). In the case of the Observability Logs Explorer this value is resolved from the setting and used as the AllSelection passed to the Logs Explorer controller.

@obltmachine
Copy link

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

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

@Kerry350 Kerry350 force-pushed the 169-use-central-log-sources-setting-in-logs-explorer branch from 03c81c0 to af94e9a Compare August 14, 2024 11:41
@Kerry350 Kerry350 force-pushed the 169-use-central-log-sources-setting-in-logs-explorer branch from af94e9a to aba77bb Compare August 15, 2024 15:49
@Kerry350
Copy link
Contributor Author

/ci

@Kerry350
Copy link
Contributor Author

/ci

@Kerry350
Copy link
Contributor Author

/ci

}

type InitialState = LogsExplorerPublicStateUpdate;
type InitialState = LogsExplorerPublicStateUpdate & { allSelection?: AllDatasetSelection };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intersection here so that LogsExplorerPublicStateUpdate maintains it's tidy 1:1 with public state.


import { AllDatasetSelection } from '../../../../common';

export const DEFAULT_ALL_SELECTION = AllDatasetSelection.create({ indices: 'logs-*-*' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in it's own file to make sure there aren't bundle size explosions from defaults.ts.

@Kerry350 Kerry350 self-assigned this Aug 20, 2024
@Kerry350 Kerry350 added release_note:skip Skip the PR/issue when compiling release notes Team:obs-onboarding Observability Onboarding Team labels Aug 20, 2024
@Kerry350 Kerry350 marked this pull request as ready for review August 20, 2024 09:19
@Kerry350 Kerry350 requested review from a team as code owners August 20, 2024 09:19
@elasticmachine
Copy link
Contributor

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

@tonyghiani tonyghiani self-requested a review August 20, 2024 09:41
@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Aug 20, 2024
@tonyghiani
Copy link
Contributor

Overall the implementation looks good, but I have some concerns about restoring the selection.
Until now, we were storing in the URL state only the selection type all, which was enough to restore the selection when loading a URL.

This has now pros and cons, as the selected index pattern is not hard-coded anymore but we rely on the user setting:

  • Pro: when a user updates the log source setting, a simple reload on the page will reflect the changes and load the right selection.
  • Cons: a user saves a URL for future inspection or simply for sharing. If the setting is then changed, that URL becomes unreliable, as the user won't be able to inspect anymore the data source that back in time has been saved for future reference.

This might complicate things a bit, I guess it depends on what is more important, although we must admit that this makes it tricky to inspect logs anytime we get a URL with the all-log selection.

IMO, restoring a URL should keep the current data source selection, including the indices, as it acts as a source of truth for the user, while it should restore the user settings when the URL has no saved state, wdyt?

@Kerry350
Copy link
Contributor Author

Kerry350 commented Aug 20, 2024

@tonyghiani

IMO, restoring a URL should keep the current data source selection, including the indices, as it acts as a source of truth for the user, while it should restore the user settings when the URL has no saved state, wdyt?

Yeah, so I went back and forth on this and went with this approach. However, there's pros and cons to both as you've laid out. My leaning was that "all" should always map to "all" and users would probably drill down (select a specific dataset) for link sharing.

This follows the precedent set by things like the log stream where if the link contains logView=(logViewId:default,type:log-view-reference) (as an example) that setting can technically change between link views. This behaviour matches more closely, but isn't necessarily a reason to do it.

Equally, we could store the indices in the URL and hydrate from that. And if the user explicitly clicks "Show all logs" it would switch back to the setting value.

Edit: I guess there'd also be some UI considerations. E.g if this is a hydrated version can we really show "All logs" for the button, it's all logs at the time then, so we'd probably need to list the indices in some way. So there are additional concerns. Storing the indices will also complicate the locator usages, as they'll also need to resolve the setting.

Honestly, I'm not strongly drawn to either, they both have merits. @flash1293 Would you like to make a judgement call?  😁

@flash1293
Copy link
Contributor

flash1293 commented Aug 22, 2024

I think it's OK to not persist the indices, but reference the setting for sharing - this is also how data views work.

@Kerry350
Copy link
Contributor Author

@tonyghiani Thanks for the review. Based on the comment from Joe regarding the URL behaviour (keeping it as is), this should be ready for another look 👀

@kibana-ci
Copy link

kibana-ci commented Aug 22, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
logsExplorer 616 617 +1
observabilityLogsExplorer 201 202 +1
total +2

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
logsExplorer 117 122 +5

Async chunks

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

id before after diff
logsExplorer 223.5KB 224.2KB +742.0B
observabilityLogsExplorer 143.2KB 143.9KB +684.0B
total +1.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
logsExplorer 22 23 +1

Page load bundle

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

id before after diff
logsExplorer 27.7KB 28.0KB +270.0B
observabilityLogsExplorer 14.4KB 14.4KB -1.0B
total +269.0B
Unknown metric groups

API count

id before after diff
logsExplorer 117 122 +5

async chunk count

id before after diff
logsExplorer 5 6 +1

History

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

cc @Kerry350

Copy link
Contributor

@tonyghiani tonyghiani 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 addressing the comments, good work here!

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

.tsconfig changes LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR 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-onboarding Observability Onboarding Team v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants