-
Notifications
You must be signed in to change notification settings - Fork 0
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
Changes from 13 commits
b95506e
6e31c3c
fbf5932
21a51d3
b4e4e46
67718cb
561bf8a
fedbe10
111a2fa
85c91bf
197bb5e
f673045
f4cd84a
11228dd
78aac1a
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 |
---|---|---|
|
@@ -43,6 +43,7 @@ | |
"Spanset", | ||
"tempopb", | ||
"querysharding", | ||
"queryless" | ||
"queryless", | ||
"lokiexplore" | ||
] | ||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]" | ||
} |
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))); | ||
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 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.movAlso on the video somehow the range was not set (it shows 1970...) but I can't reproduce it anymore 😅 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. 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. 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. 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? 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. 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 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.
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} />; | ||
}; | ||
|
@@ -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 | ||
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.
Does it mean other apps could potentially call 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. Yes, it's not ideal and it's also just temporary. The reason for it is that otherwise you have to bound Once this is good enough the |
||
* 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 { | ||
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. 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 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. Those are 2 possibilities, not sure which makes more sense.
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
"type": "app", | ||
"name": "Explore Traces", | ||
"id": "grafana-exploretraces-app", | ||
"preload": true, | ||
"autoEnabled": true, | ||
"info": { | ||
"keywords": ["app", "tempo", "traces", "explore"], | ||
|
@@ -54,5 +55,14 @@ | |
"dependencies": { | ||
"grafanaDependency": ">=11.2.0", | ||
"plugins": [] | ||
}, | ||
"extensions": { | ||
"addedLinks": [ | ||
{ | ||
"targets": ["grafana-lokiexplore-app/toolbar-open-related/v1"], | ||
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. 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.
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. 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 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.
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? 🤔 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. 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" | ||
} | ||
] | ||
} | ||
} |
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 checked and it doesn't seem like we need this file or react-router here at all. We can just render
TraceExplorationPage
insideApp
instead of theRoutes
and it should work. The actual plugin routes are registered via plugin.json'sincludes
property, so I think this explicit route declaration is redundant.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.
Yeah could be but don't want to do too many unrelated changes here so will leave that for traces team to handle separately.