Skip to content
This repository has been archived by the owner on Jul 19, 2023. It is now read-only.

Make both /ui/?query= and /ui?query= work #649

Merged
merged 3 commits into from
Apr 28, 2023

Conversation

Rustin170506
Copy link
Contributor

Follow up #645

Test locally:

  • Before change

image

  • After change

image

Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

I would prefer to redirect /ui to /ui/

@Rustin170506
Copy link
Contributor Author

I would prefer to redirect /ui to /ui/

Usually, we use /ui?a=xxx instead of /ui/?a=xxx. For example: https://github.com/issues?q=is%3Aopen+is%3Aissue+author%3Ahi-rustin+archived%3Afalse+phlare.

I think adding a redirection here is a little bit weird. It is not common to have this kind of redirection.

@simonswine
Copy link
Collaborator

I don't think it is weird, usually this redirection is done to support relative linking of assets and APIs.

Maybe an example to make it clearer:

  • User wants Phlare to mount a via a reverse proxy in there https://tooling.corp/phlare
  • When you are accessing it using /phlare/ui, the API will be under ../api
  • When you are accessing it using /phlare/ui/, the API will be under ../../api.

Thanks why I think a redirect is better in this case. And we would always be at the same "depth" for relative linking

cc @eh-am

@eh-am
Copy link
Collaborator

eh-am commented Apr 26, 2023

@simonswine's reasoning is sound, another way of looking at this is that the entire /ui/ subpath is its own "app", where / is the root of that app.

Honestly I didn't do any work here hoping that we would default the UI to the root instead of /ui/ once most pages were ready. But oh well.

@Rustin170506

This comment was marked as off-topic.

@Rperry2174
Copy link
Contributor

Off-topic: I would like to ask what is the relationship between this UI and the UI in grafana? Will they be merged? And what is the relationship with pyroscope UI?

We will provide more official guidance in this at our next community meeting / via other channels.

However, just so you are in the loop as you've been helping us with this (thanks btw!) our long term goal is merging the Pyroscope and Phlare projects.

Once they are merged there will likely be two ways to view profiling data:

  1. Via standalone UI (the one being talked about here)
  2. Via Grafana UI (explore tab and deeper integrations into Grafana)

In the meantime, we've made this UI compatible with both phlare and pyroscope projects to unify the frontend which users interact with while we work on merging the backend.

That way when the projects are eventually merged it will be done in the "background" with minimum disruption to users (from a UI perspective)

@Rustin170506

This comment was marked as off-topic.

@Rustin170506 Rustin170506 requested a review from simonswine April 27, 2023 02:25
@Rustin170506
Copy link
Contributor Author

I don't think it is weird, usually this redirection is done to support relative linking of assets and APIs.

Maybe an example to make it clearer:

  • User wants Phlare to mount a via a reverse proxy in there https://tooling.corp/phlare
  • When you are accessing it using /phlare/ui, the API will be under ../api
  • When you are accessing it using /phlare/ui/, the API will be under ../../api.

Sounds reasonable. Thanks for your review!

Honestly I didn't do any work here hoping that we would default the UI to the root instead of /ui/ once most pages were ready. But oh well.

Make sense. I also prefer to move UI to the root and move admin pages to /admin or other routes. It makes accessing the data easier.

Copy link
Collaborator

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/api/api.go Outdated Show resolved Hide resolved
@simonswine simonswine merged commit 90c8e24 into grafana:main Apr 28, 2023
simonswine pushed a commit to simonswine/pyroscope that referenced this pull request Jun 30, 2023
* Make both `/ui/?query=` and `/ui?query=` work

* Add redirection

* Address comment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants