-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Logs and Metrics UI] Initial setup for registering observability overview data fetchers #69999
[Logs and Metrics UI] Initial setup for registering observability overview data fetchers #69999
Conversation
4c885aa
to
9c64ad8
Compare
@@ -21,7 +21,7 @@ import { PluginInitializerContext } from '../../../core/public'; | |||
import { UsageCollectionPlugin } from './plugin'; | |||
|
|||
export { METRIC_TYPE } from '@kbn/analytics'; | |||
export { UsageCollectionSetup } from './plugin'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is here because I missed exporting it when I updated the UsageCollection plugin start contract here: #69836
@@ -12,7 +12,7 @@ import { | |||
KibanaContextProvider, | |||
} from '../../../../../src/plugins/kibana_react/public'; | |||
import { TriggersActionsProvider } from '../utils/triggers_actions_context'; | |||
import { ClientPluginDeps } from '../types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed some names here to try to make it all consistent. We now have most of our types prefaced with "InfraClient" (eventually to be LogsClient and MetricsClient, with "Client" to distinguish from the "Server" types), and then we use StartDeps/StartExports and SetupDeps/SetupExports to specify the dependencies and exports coming and going out of each lifecycle method.
InfraClientStartExports, | ||
InfraClientSetupDeps, | ||
InfraClientStartDeps, | ||
} from './types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9c64ad8
to
a6e24af
Compare
a6e24af
to
ef9bcc6
Compare
// perform query | ||
return { | ||
title: 'Log rate', | ||
appLink: 'TBD', // TODO: what format should this be in, relative I assume? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think for the sake of simplicity observability plugin should handle the relative part while generating link.
When I try to start this PR I get these errors:
It looks like if we changed |
@cauemarcondes can you take a look at the change I made in the observability plugin to properly export the new types? @simianhacker has a comment above about improving what I did so it works, not sure why we needed a |
import { InfraClientCoreSetup } from '../types'; | ||
import { LogsFetchDataResponse } from '../../../observability/public'; | ||
|
||
export function getLogsHasDataFetcher(getStartServices: InfraClientCoreSetup['getStartServices']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cauemarcondes I understand this function is a requirement for the dashboard. Is it a hard requirement?
I can imagine that the reason to have it is to not perform a potentially expensive query to fetch the data if there's no data to fetch.
if that's the case, how expensive would it be to call the data fetching function when there's no data at all, versus calling two functions every time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afgomez, the idea behind the hasData
function is indeed to get a flag indicating if there's any data available in the logs
indices. This exists because we have two pages in the Observability UI /landing
and /overview
plus /
. The /
URL, will call the hasData
from all plugins, if one of the results is true
we redirect the user to /overview
otherwise /landing
.
We could, of course, call the fetchData
instead of hasData
, but in that case, the response time would probably be twice or even thrice time slower then only checking if there's data available.
Also hasData
must return false, if a user doesn't have privileges to read from the logs indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is good 👍.
@cauemarcondes I left a comment regarding something I assume is a requirement from the observability plugin side, but it's not blocking to merge this.
@@ -6,3 +6,4 @@ | |||
|
|||
export * from './eui_draggable'; | |||
export * from './eui_styled_components'; | |||
export * from './fetch_data_response'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasonrhodes That looks good to me, I'm just curious to understand why the current way doesn't work for you. I am importing it on APM, and it works. https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/public/services/rest/observability_dashboard.ts#L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, @cauemarcondes -- I'm surprised you're not getting a linting error there. Just 2 lines above that you have the "// eslint-disable-next-line @kbn/eslint/no-restricted-paths" comment which is exactly about this same thing. My understanding is you can only import from plugin/public or plugin/server when importing from another plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, @cauemarcondes -- I'm surprised you're not getting a linting error there. 2 lines above you have "// eslint-disable-next-line @kbn/eslint/no-restricted-paths" which is about this very thing. My understanding is you can only import from plugin/public or plugin/server, when importing from other plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I had to rename fetch_data_response/index.d.ts
to index.ts
(note the lack of .d
). Otherwise webpack complained when trying to create the bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes for Kibana-Telemetry LGTM (only exporting UsageCollectionStart
in src/plugins/usage_collection/public/index.ts
)
Thanks @jasonrhodes
…odes/kibana into logs-data-for-obs-homepage_68531
Pinging @elastic/apm-ui (Team:apm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 from ingest management
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
…rview data fetchers (elastic#69999) * Switches mount callbacks to only use start deps Fixes elastic#58014 * Sets up skeleton logs data fetchers for overview * Fixes type hacks for logs fetcher * Prevent kibana from crashing on initial load * Fixes types and linting errors * Fixes some linting import/export issues Co-authored-by: Alejandro Fernández Gómez <alejandro.fernandez@elastic.co>
…rview data fetchers (#69999) (#70351) * Switches mount callbacks to only use start deps Fixes #58014 * Sets up skeleton logs data fetchers for overview * Fixes type hacks for logs fetcher * Prevent kibana from crashing on initial load * Fixes types and linting errors * Fixes some linting import/export issues Co-authored-by: Alejandro Fernández Gómez <alejandro.fernandez@elastic.co> Co-authored-by: Alejandro Fernández Gómez <alejandro.fernandez@elastic.co>
…rview data fetchers (elastic#69999) * Switches mount callbacks to only use start deps Fixes elastic#58014 * Sets up skeleton logs data fetchers for overview * Fixes type hacks for logs fetcher * Prevent kibana from crashing on initial load * Fixes types and linting errors * Fixes some linting import/export issues Co-authored-by: Alejandro Fernández Gómez <alejandro.fernandez@elastic.co>
This implements the basic skeleton for Logs and Metrics to register the Observability Homepage data retrieval functions.