-
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
Improve functionality when opened inside sidecar #264
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the updated PR. Few things here I think should be fixed before we proceed.
-
View linked span on the service structure tab does not work in sidecar.
-
Opening up Explore Traces in sidecar still creates an invalid query.
-
When sidecar is opened, the
Related traces
button is missing. If the user wants to update the context in sidecar because they have selected a new label in Loki they cannot.
- The top padding on the need help and time range buttons are reduced when in sidecar
-
Can we add a tooltip to the x in the sidecar view that says something like
Close sidecar
. Multiple close buttons which are near each other are a bit confusing from a UX perspective. -
Copying the URL via the share button opens the same view twice, not sure what the best approach here is but this can be improved. Sidecars context is not in the url. This is different from how state / sharing / bookmarks is otherwise handled in explore/apps/dashboards. Could you please explain why this is the approach? cc @adrapereira if you have more context on state in the url.
this.state.body.setState({ secondary: detailsScene }); | ||
} | ||
const detailsScene = this.state.detailsScene?.resolve(); | ||
this.setSecondaryScene(detailsScene) |
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.
missing semicolon
// The API looks a bit weird because duh we are opened if we are here, but this specifically means we are in a | ||
// sidecar with some other app. In that case we don't want to show additional split layout as there is not much | ||
// space and 3 splits is a bit too much. | ||
const body: SceneObject = sidecarServiceSingleton_EXPERIMENTAL?.isAppOpened(pluginJson.id) ? singleBody : splitBody; |
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.
Can we also check the feature toggle is enabled?
Good catch, fixed.
So not really going to fix it here. Like the discussion about how to normalize labels between apps/backend is bit complicated. Metrics for example have a whole project about how to mat their label to logs so this is a bit out of my domain knowledge right now and will have to be figured out later on.
So at first I was thinking about this as just "Open" button so it seems reasonable to not show it if Traces are already opened but as you suggest this actually works for sort of state update. So will leave it visible for now, in the future though I assume the button will be in some submenu (cause there would related metrics and profiles) so at that point we will have to figure out some better way to sync labels after changes.
I actually don't see it locally nor in ops so not sure what is going on. Don't think the sidecar itself should change any styles of the app inside it. Do you see it in ops?
Yes but that is part of Grafana code so will have to do it there later.
So yeah this is something I am working on and figuring out exact rules. In the long run I think there would be 2 kinds of apps one that will have state in the URL like this but also ones that don't like the Explorations app which does not make sense to be shared in the URL. Having state of sidecar in the URL just isn't implemented yet so this uses the non URL mode but hope to change that soon. But it's also something to be changed in the Grafana code. |
Main things being changed here: