-
Notifications
You must be signed in to change notification settings - Fork 13
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
Exposed service logs view #498
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,76 @@ | ||
import { AdHocVariableFilter, TimeRange, dateTimeParse } from "@grafana/data"; | ||
import { SceneTimeRange } from '@grafana/scenes'; | ||
import { IndexScene } from "Components/IndexScene/IndexScene"; | ||
import { isEqual } from "lodash"; | ||
import React, { useEffect, useMemo } from "react"; | ||
|
||
const DEFAULT_TIME_RANGE = { from: 'now-15m', to: 'now' }; | ||
|
||
interface Props { | ||
initialFilters?: AdHocVariableFilter[]; | ||
initialDS?: string; | ||
initialTimeRange?: TimeRange; | ||
onTimeRangeChange?: (timeRange: TimeRange) => void; | ||
} | ||
|
||
export function ExposedLogExplorationPage({ initialFilters, initialDS, initialTimeRange, onTimeRangeChange }: Props) { | ||
const [isInitialized, setIsInitialized] = React.useState(false); | ||
// Must memoize the top-level scene or any route change will re-instantiate all the scene classes | ||
const scene = useMemo( | ||
() => | ||
new IndexScene({ | ||
$timeRange: new SceneTimeRange( | ||
initialTimeRange | ||
? { | ||
from: | ||
typeof initialTimeRange.raw.from === 'string' | ||
? initialTimeRange.raw.from | ||
: initialTimeRange.raw.from.toISOString(), | ||
to: | ||
typeof initialTimeRange.raw.to === 'string' | ||
? initialTimeRange.raw.to | ||
: initialTimeRange.raw.to.toISOString(), | ||
} | ||
: DEFAULT_TIME_RANGE | ||
), | ||
$behaviors: [ | ||
(scene: IndexScene) => { | ||
const unsubscribable = scene.state.$timeRange?.subscribeToState((newState, oldState) => { | ||
if (!isEqual(newState, oldState)) { | ||
const timeRange: TimeRange = { | ||
from: dateTimeParse(newState.from), | ||
to: dateTimeParse(newState.to), | ||
raw: { | ||
from: newState.from, | ||
to: newState.to, | ||
}, | ||
}; | ||
onTimeRangeChange?.(timeRange); | ||
} | ||
}); | ||
|
||
return () => { | ||
unsubscribable?.unsubscribe(); | ||
}; | ||
}, | ||
], | ||
initialFilters, | ||
initialDS, | ||
mode: 'service_details', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a prop? |
||
}), | ||
//eslint-disable-next-line react-hooks/exhaustive-deps | ||
[] | ||
); | ||
|
||
useEffect(() => { | ||
if (!isInitialized) { | ||
setIsInitialized(true); | ||
} | ||
}, [scene, isInitialized]); | ||
|
||
if (!isInitialized) { | ||
return null; | ||
} | ||
|
||
return <scene.Component model={scene} />; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,15 @@ | ||
import { AppPlugin } from '@grafana/data'; | ||
import { App } from 'Components/App'; | ||
import { ExposedLogExplorationPage } from 'Components/ExposedLogExplorationPage/v1/ExposedLogExplorationPage'; | ||
import pluginJson from 'plugin.json'; | ||
|
||
export const plugin = new AppPlugin<{}>().setRootPage(App); | ||
|
||
if (plugin.exposeComponent) { | ||
plugin.exposeComponent({ | ||
id: `${pluginJson.id}/explore-logs/v1`, | ||
component: ExposedLogExplorationPage, | ||
title: 'Explore logs view', | ||
description: 'Explore logs service details view for grafana plugins to consume.', | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
"type": "app", | ||
"name": "Logs", | ||
"id": "grafana-lokiexplore-app", | ||
"preload": true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think so. It doesn't work for me without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, might need some insights from @leventebalogh, also whether we need to specify the extension in the plugin.json There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please take a look at this thread before proceeding with https://raintank-corp.slack.com/archives/C077S1MP40M/p1719937543634869 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @leeoniya, looks like no need for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately we currently need the
Yeah, I agree that this would be ideal, however unfortunately we cannot avoid it for now. Regarding undoing it later: once the PR mentioned above is merged
Yes, that would be great, thanks. (We can only preload plugins properly if we know what they are exposing.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @leventebalogh ! So is this PR just a prerequisite? Or is it for Grafana extensions only, not plugin extensions?
Makes me think that the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly @jarben, that PR was only a prerequisite, it still relies on
Yea, I guess plugins like like this will still need to be preloaded ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @leventebalogh, great to see the cloud-home-app exception was considered in the PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly, it's the third-party library that it is using for A/B testing. (The issue is that it's somehow causing a re-rendering of the whole |
||
"info": { | ||
"keywords": ["app", "loki", "explore", "logs"], | ||
"description": "Query-less exploration of log data stored in Loki", | ||
|
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.
What happens when this is empty when optional?
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 a really good question, and I would say a more global one.
Explore logs app has 2 modes:
'service_selection' | 'service_details'
. Not sure I can imagine the situation where we need to "embed" and start fromservice_selection
view. So my initial idea was to use onlyservice_details
view with a particularservice_name
filter filled (this is a condition to have this mode opened, otherwise explore logs app redirects toservice_selection
). I am thinking about sticking to this (and this made me think about some other changes - like I need to disable removingservice_name
filter for example) wdyt @jarben ?so answering your question:
initialFilters
are empty and the mode isservice_details
currently it just redirects to explore logs app withservice_selection
view enabled.initialDS
is empty it applies the default one.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, thanks for all the details @kozhuhds! So the question is whether to make the extension generic or specific to a service?
mode
as a param would make sense so users can explicitly chooseservice_selection
if that's what they want.explore-service-logs/v1
and make theservice_name
param required? (we can also make an extension such asexplore-services
)