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

Exposed service logs view #498

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,10 @@
},
"dependencies": {
"@emotion/css": "^11.10.6",
"@grafana/data": "^11.0.0",
"@grafana/runtime": "^11.0.0",
"@grafana/data": "^11.2.0-183075",
"@grafana/runtime": "^11.2.0-183075",
"@grafana/scenes": "4.32.0",
"@grafana/ui": "^11.0.0",
"@grafana/ui": "^11.2.0-183075",
"@playwright/test": "^1.43.1",
"@types/react-table": "^7.7.20",
"react": "18.2.0",
Expand Down
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,
Copy link

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?

Copy link
Author

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 from service_selection view. So my initial idea was to use only service_details view with a particular service_name filter filled (this is a condition to have this mode opened, otherwise explore logs app redirects to service_selection). I am thinking about sticking to this (and this made me think about some other changes - like I need to disable removing service_name filter for example) wdyt @jarben ?

so answering your question:

  1. If initialFilters are empty and the mode is service_details currently it just redirects to explore logs app with service_selection view enabled.
  2. If initialDS is empty it applies the default one.

Copy link

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?

  • If generic, making the mode as a param would make sense so users can explicitly choose service_selection if that's what they want.
  • if service-specific, what if we rename the extension to explore-service-logs/v1 and make the service_name param required? (we can also make an extension such as explore-services)

mode: 'service_details',
Copy link

Choose a reason for hiding this comment

The 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} />;
}
11 changes: 11 additions & 0 deletions src/module.ts
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.',
});
}
1 change: 1 addition & 0 deletions src/plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"type": "app",
"name": "Logs",
"id": "grafana-lokiexplore-app",
"preload": true,
Copy link

Choose a reason for hiding this comment

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

Do we need preload:true when exposing components? (I used to think this was for non-hook component extensions only)

Copy link
Author

Choose a reason for hiding this comment

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

yeah, I think so. It doesn't work for me without preload: true.

Copy link

Choose a reason for hiding this comment

The 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

Copy link

@leeoniya leeoniya Jul 2, 2024

Choose a reason for hiding this comment

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

please take a look at this thread before proceeding with preload: true. ideally we can avoid introducing this rather than trying to undo it later.

https://raintank-corp.slack.com/archives/C077S1MP40M/p1719937543634869

Copy link

Choose a reason for hiding this comment

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

Thanks @leeoniya, looks like no need for preload: true is supported in 11.1.0, and it requires the plugin.json metadata as mentioned by @sympatheticmoose in the thread:

we'd tried a metadata extractor approach which proved unviable, but theres alternatives by just having the information manually added to plugin.json etc

Choose a reason for hiding this comment

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

@jarben @leeoniya

Unfortunately we currently need the preload: true flag. It's true, that we are working on only loading plugins when they are used (extensions or normal plugin usage), but the PR is not yet merged and deployed.

ideally we can avoid introducing this rather than trying to undo it later.

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 preload: true won't have an effect, which means that even though we still need to do clean ups in the plugin.json, it won't affect performance.

we need to specify the extension in the plugin.json

Yes, that would be great, thanks. (We can only preload plugins properly if we know what they are exposing.)

Copy link

Choose a reason for hiding this comment

The 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?

once the grafana/grafana#86624 is merged preload: true won't have an effect, which means that even though we still need to do clean ups in the plugin.json, it won't affect performance.

Makes me think that the cloud-home-app also uses preload:true to be able to show notification banners anywhere in Grafana, not just after the plugin is loaded. How would we handle this use case if preload:true doesn't work anymore?

Choose a reason for hiding this comment

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

Exactly @jarben, that PR was only a prerequisite, it still relies on preload: true (here), but the preloading doesn't block the Grafana app boot process anymore. :)

Makes me think that the cloud-home-app also uses preload:true to be able to show notification banners anywhere in Grafana, not just after the plugin is loaded. How would we handle this use case if preload:true doesn't work anymore?

Yea, I guess plugins like like this will still need to be preloaded (cloud-home-app is an exception already). What would be good if they wouldn't block the boot process at least, but unfortunately we couldn't do that with the cloud-home-app, as one of the external libraries it's using is causing weird bugs if it is initialised after Grafana has loaded.

Copy link

Choose a reason for hiding this comment

The 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.
Do you remember which library caused the issue? @grafana/cloud-ab-testing?

Choose a reason for hiding this comment

The 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 AppChrome in case it's not loaded before Grafana boots up. I was trying to debug it, but couldn't find the root cause during a time-box.)

"info": {
"keywords": ["app", "loki", "explore", "logs"],
"description": "Query-less exploration of log data stored in Loki",
Expand Down
Loading
Loading