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

Sidecar: Implement basic integration with Explore Logs #102

Merged
merged 15 commits into from
Nov 12, 2024
Merged
3 changes: 2 additions & 1 deletion cspell.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
"Spanset",
"tempopb",
"querysharding",
"queryless"
"queryless",
"lokiexplore"
]
}
853 changes: 384 additions & 469 deletions package-lock.json

Large diffs are not rendered by default.

9 changes: 5 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,16 +72,17 @@
},
"dependencies": {
"@emotion/css": "^11.10.6",
"@grafana/data": "^11.2.0",
"@grafana/runtime": "^11.2.0",
"@grafana/data": "^11.3.0",
"@grafana/runtime": "^11.3.0",
"@grafana/scenes": "^5.14.7",
"@grafana/ui": "11.2.0",
"@grafana/ui": "11.3.0",
"react": "18.2.0",
"react-dom": "18.2.0",
"react-loading-skeleton": "3.4.0",
"react-router-dom": "^5.2.0",
"rxjs": "7.8.0",
"tslib": "2.5.3"
"tslib": "2.5.3",
"zod": "^3.23.8"
},
"packageManager": "[email protected]"
}
5 changes: 3 additions & 2 deletions src/components/Routes/Routes.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import React from 'react';
import { Redirect, Route, Switch } from 'react-router-dom';
import { Redirect, Switch } from 'react-router-dom';
import { CompatRoute } from 'react-router-dom-v5-compat';
import { TraceExplorationPage } from '../../pages/Explore';
import { PLUGIN_BASE_URL, ROUTES } from 'utils/shared';

export const Routes = () => {
return (
<Switch>
<Route path={prefixRoute(`${ROUTES.Explore}`)} component={TraceExplorationPage} />
<CompatRoute path={prefixRoute(`${ROUTES.Explore}`)} component={TraceExplorationPage} />
<Redirect to={prefixRoute(ROUTES.Explore)} />
</Switch>
);

Choose a reason for hiding this comment

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

I checked and it doesn't seem like we need this file or react-router here at all. We can just render TraceExplorationPage inside App instead of the Routes and it should work. The actual plugin routes are registered via plugin.json's includes property, so I think this explicit route declaration is redundant.

Copy link
Member Author

@aocenas aocenas Nov 11, 2024

Choose a reason for hiding this comment

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

Yeah could be but don't want to do too many unrelated changes here so will leave that for traces team to handle separately.

Expand Down
33 changes: 27 additions & 6 deletions src/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,30 @@ import { AppPlugin } from '@grafana/data';
import { App } from './components/App';
import { AppConfig } from './components/AppConfig';

export const plugin = new AppPlugin<{}>().setRootPage(App).addConfigPage({
title: 'Configuration',
icon: 'cog',
body: AppConfig,
id: 'configuration',
});
// @ts-ignore new API that is not yet in stable release
import { sidecarServiceSingleton_EXPERIMENTAL } from '@grafana/runtime';
import pluginJson from './plugin.json';

export const plugin = new AppPlugin<{}>()
.setRootPage(App)
.addConfigPage({
title: 'Configuration',
icon: 'cog',
body: AppConfig,
id: 'configuration',
})
.addLink({
title: 'traces',
description: 'Open traces',
icon: 'align-left',
targets: 'grafana-lokiexplore-app/toolbar-open-related/v1',
configure: () => {
if (sidecarServiceSingleton_EXPERIMENTAL?.isAppOpened(pluginJson.id)) {
return undefined;
}
return {};
},
onClick: (e, helpers) => {
sidecarServiceSingleton_EXPERIMENTAL?.openApp(pluginJson.id, helpers.context);
},
});
39 changes: 38 additions & 1 deletion src/pages/Explore/TraceExplorationPage.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,23 @@
import React, { useEffect, useState } from 'react';
import z from 'zod';

import { newTracesExploration } from '../../utils/utils';
import { TraceExploration } from './TraceExploration';
import { DATASOURCE_LS_KEY } from '../../utils/shared';
import { reportAppInteraction, USER_EVENTS_ACTIONS, USER_EVENTS_PAGES } from '../../utils/analytics';
import { UrlSyncContextProvider } from '@grafana/scenes';
import { AdHocVariableFilter } from '@grafana/data';

// @ts-ignore new API that is not yet in stable release
import { useSidecar_EXPERIMENTAL } from '@grafana/runtime';

export const TraceExplorationPage = () => {
// We are calling this conditionally, but it will depend on grafana version and should not change in runtime so we
// can ignore the hook rule here
const sidecarContext = useSidecar_EXPERIMENTAL?.() ?? {};

const initialDs = localStorage.getItem(DATASOURCE_LS_KEY) || '';
const [exploration] = useState(newTracesExploration(initialDs));
const [exploration] = useState(newTracesExploration(initialDs, getInitialFilters(sidecarContext.initialContext)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we clear the context after the app is opened? At the moment if the side panel is not closed with "x" it just stays open. It's stored in the local storage so it opens in new tabs as well:

Screen.Recording.2024-11-05.at.12.16.25.mov

Also on the video somehow the range was not set (it shows 1970...) but I can't reproduce it anymore 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is the kind of feedback I hope for after this is in Ops. The thing is the persistence was added because the Investigations app which makes sense to be kept "alive" as you can add multiple things to it from multiple places. Right now not sure what exactly is the cases where that is not wanted. Maybe we can have it somehow configurable but not sure what exactly the options should be right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this can go like this for testing internally, though it would be nice to make the "X" button more visible somehow or maybe sticky for now? I'm worried some user may be not sure how to get rid of this. Also it may be confusing as it may show up on any tab after it's open, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made some changes so the width of the header with the X button is fixed so it never goes out of sight. Later will probably try to design it a bit better. For example I assume it needs some sort of additional action, like "maximize" or "open in separate tab" etc

Copy link
Member Author

Choose a reason for hiding this comment

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

Also it may be confusing as it may show up on any tab after it's open, right?

Yes the state is persisted. Again not sure what is the best approach here, at some point I will make it configurable as part of also making the URL work properly, so that explore apps will just be persisted in the URL and work sort of normally and having a non URL but persisted in local storage state as some sort of opt in for specific apps.


return <TraceExplorationView exploration={exploration} />;
};
Expand All @@ -33,3 +43,30 @@ export function TraceExplorationView({ exploration }: { exploration: TraceExplor
</UrlSyncContextProvider>
);
}

const AdHocVariableFilterSchema = z.object({
key: z.string(),
operator: z.string(),
value: z.string(),
});

const InitialFiltersSchema = z.object({
filters: z.array(AdHocVariableFilterSchema),
});

/** Because the context comes from a different app plugin we cannot really count on it being the correct type even if
* it was typed, so it is safer to do runtime parsing here. It also can come from different app extensions and at this
Copy link
Collaborator

Choose a reason for hiding this comment

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

It also can come from different app extensions...

Does it mean other apps could potentially call sidecarServiceSingleton_EXPERIMENTAL?.openApp(anyPluginIdHere, helpers.context); providing any id in anyPluginIdHere?

Copy link
Member Author

@aocenas aocenas Nov 5, 2024

Choose a reason for hiding this comment

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

Yes, it's not ideal and it's also just temporary. The reason for it is that otherwise you have to bound openApp to current app plugin which can only been done by propagating it through some function args or props, but that then means this would already change existing API which means less space for exploration and changes later on.

Once this is good enough the sidecarServiceSingleton_EXPERIMENTAL APIs will probably be moved elsewhere to accommodate this.

* point we don't know which, but we also have implemented only one so far it's a fair guess.
*
* At this point there is no smartness. What ever we got from the other app we use as is. Ideally there should be some
* normalization of the filters or smart guesses when there are differences.
* @param context
*/
function getInitialFilters(context: unknown): AdHocVariableFilter[] | undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the plan to use AdHocFilters as common contract for context between the apps? Or it's assumed that if I'm creating a hook for grafana-lokiexplore-app/toolbar-open-related/v1" I should check what type of context it provides?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are 2 possibilities, not sure which makes more sense.

  1. Having a sync button on AdHocFilters could be nice (in similar way that it is right now with timePickers) but I think you may want to add some additional processing as lot's of times the labels aren't exactly the same.
  2. Leave it to the application, so they can use some other way to stay sync (extension for that or some event bus). I think this depends on whether we can find good enough standardization or not.

Regarding the context check, I think that will be necessary. Even if we add some common types in some grafana/* package you could use, it still does not make sense to blindly trust the data, because this is a boundary that you just cannot compile time check if the types are right in the same way you cannot check network request at compile times.

const result = InitialFiltersSchema.safeParse(context);
if (!result.success) {
return undefined;
}

return result.data.filters;
}
10 changes: 10 additions & 0 deletions src/plugin.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"type": "app",
"name": "Explore Traces",
"id": "grafana-exploretraces-app",
"preload": true,
"autoEnabled": true,
"info": {
"keywords": ["app", "tempo", "traces", "explore"],
Expand Down Expand Up @@ -54,5 +55,14 @@
"dependencies": {
"grafanaDependency": ">=11.2.0",
"plugins": []
},
"extensions": {
"addedLinks": [
{
"targets": ["grafana-lokiexplore-app/toolbar-open-related/v1"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This creates a link to very specific place in the UI. Would it mean we need to create multiple links to other apps or we're also planning to make more generic extension point that apps could reuse? e.g.

  • extension point grafana/side-car
  • the app with the button calls <SideCarDropDown ... > that renders all registered hooks

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm not sure exactly what you mean. Like extension point is always a specific place in the UI by design. You could do something like grafana/side-car and then have that extension point on multiple places but then you loose the information about the location which may be important (ie whether it's in logs or metrics app), so you probably again end up with some way to differentiate them which could already be the ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Like extension point is always a specific place in the UI by design.

The confusing part to me is that if we have specific link and we know where the action is coming from - why do we need to figure out the context and check the schema? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, okay, I just saw you comment below "Regarding the context check, I think that will be necessary....". Fair enough. I thought the "guessing" logic would be to support multiple inputs not assert the expected contract 👍

"title": "traces",
"description": "Open traces"
}
]
}
}
4 changes: 2 additions & 2 deletions src/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ export function getTraceByServiceScene(model: SceneObject): TracesByServiceScene
return sceneGraph.getAncestor(model, TracesByServiceScene);
}

export function newTracesExploration(initialDS?: string): TraceExploration {
export function newTracesExploration(initialDS?: string, initialFilters?: AdHocVariableFilter[]): TraceExploration {
return new TraceExploration({
initialDS,
initialFilters: [primarySignalOptions[0].filter],
initialFilters: initialFilters ?? [primarySignalOptions[0].filter],
$timeRange: new SceneTimeRange({ from: 'now-15m', to: 'now' }),
});
}
Expand Down
Loading