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

Improve functionality when opened inside sidecar #264

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aocenas
Copy link
Member

@aocenas aocenas commented Nov 11, 2024

Main things being changed here:

  1. Opening a single trace correctly references context locationService instead of the global singleton which prevented the navigation inside sidecar
  2. The detail view (like the trace view) is opened as main view, not the split view when inside the sidecar. This is to prevent triple split view in that case.
  3. Small responsive update to the main controls so they wrap around when there isn't enough space

Copy link
Collaborator

@joey-grafana joey-grafana left a 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.

Screenshot 2024-11-18 at 10 26 23

  • The top padding on the need help and time range buttons are reduced when in sidecar

Screenshot 2024-11-18 at 10 31 06

  • 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.

Screenshot 2024-11-18 at 10 45 48

Screenshot 2024-11-18 at 10 47 15

Screenshot 2024-11-18 at 10 47 24

this.state.body.setState({ secondary: detailsScene });
}
const detailsScene = this.state.detailsScene?.resolve();
this.setSecondaryScene(detailsScene)
Copy link
Collaborator

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;
Copy link
Collaborator

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?

@aocenas
Copy link
Member Author

aocenas commented Nov 18, 2024

@joey-grafana

View linked span on the service structure tab does not work in sidecar.

Good catch, fixed.

Opening up Explore Traces in sidecar still creates an invalid query.

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.

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.

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.

The top padding on the need help and time range buttons are reduced when in sidecar

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?

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.

Yes but that is part of Grafana code so will have to do it there later.

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. ...

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔍 In review
Development

Successfully merging this pull request may close these issues.

2 participants