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

[WIP]New Revamped UI in React #328

Closed
wants to merge 6 commits into from
Closed

Conversation

weastel
Copy link
Contributor

@weastel weastel commented Feb 15, 2020

Signed-off-by: Drumil Patel drumilpatel720@gmail.com
Opening this PR but it should be merge after APIs PR is merged

Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
@weastel weastel changed the title New Revamped UI in React [WIP]New Revamped UI in React Feb 15, 2020
Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
@weastel
Copy link
Contributor Author

weastel commented Feb 16, 2020

@beorn7 Why is prometheus logo blue in pushgateway? Is it intentional or it is just an old version of prometheus logo?

Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
Signed-off-by: Drumil Patel <drumilpatel720@gmail.com>
@beorn7
Copy link
Member

beorn7 commented Feb 16, 2020

Why is prometheus logo blue in pushgateway? Is it intentional or it is just an old version of prometheus logo?

It's deliberate.

It's actually a quite recent change. I got confused that my Prometheus browser tabs have the same favicon as my PGW ones. So I inverted it (push instead of pull :o). For me, it worked out quite nicely.

@beorn7
Copy link
Member

beorn7 commented Feb 16, 2020

I assume you are moving as much as possible over from the new Prometheus UI (in terms of layout but also how we solved the build process, licensing issues, and everything over there).

We should definitely get help from experts for the new Prometheus UI (like @juliusv or @boyskila ) once this is ready for a review.

@juliusv
Copy link
Member

juliusv commented Feb 16, 2020

Hi! I might have missed something, but was there a discussion and/or agreement on moving the PGW UI to React? I can't find one, at least. At first thought at least it seems a bit overkill for what the PGW does to require that sort of build infra vs. just the bit of jQuery that it currently has? Or are there plans to add a complex web app UI to the PGW?

@roidelapluie
Copy link
Member

roidelapluie commented Feb 16, 2020

I agree that react has serious disadvantages like the assets that are not deterministic. While prometheus server has enough eyes on it and kinda accepted that fact, I am not sure that we want it for smaller repos.

@beorn7
Copy link
Member

beorn7 commented Feb 16, 2020

No discussion has happened, and certainly no decision.

Course of action:

There were several requests for an HTTP API for the pushed metrics, with the primary idea to use it by 3rd party services (i.e. not with UI in mind). - @drumilpatel2000 is currently working on said API in #327 . However, once the API is in place, it seems reasonable to let the UI be driven by the API rather than server-side Go templating (which inevitably repeats the same logic as the HTTP API).

That's where we are. The current PGW UI is (visually) modelled after the (classic) Prometheus UI, just for the unified experience. With Prometheus server moving to react, the question is how far and how deep the PGW UI should be modelled on that, too. @drumilpatel2000 in this WIP PR started to model it pretty deeply on Prometheus. We now need exactly the feedback you started to provide.

@juliusv
Copy link
Member

juliusv commented Feb 17, 2020

My idea / hope was that in the long run, the two components with complex web UIs would convert to React: Prometheus and Alertmanager, with the first one already being done. React really helps when you want to manage complex application state over time, and keep the state consistent and the codebase manageable / understandable. But it comes with a huge amount of new concepts and a more convoluted build infrastructure that IMO is only worth it in those cases. I'm much less sure in the case of the Pushgateway, which basically shows completely static info (modulo simple expander buttons).

I do see the slight duplication argument with the introduction of the new API, but I think that's still worth it over introducing something as heavy-weight as React, unless you expect the PGW UI to either change or expand a lot in the future.

@beorn7
Copy link
Member

beorn7 commented Feb 17, 2020

The PGW UI has more than just static info. It allows to delete all or parts of the pushed metrics. But it doesn't really have to manage complex state.

Thanks @juliusv for the valuable feedback. I wasn't following closely how the React part evolved. If the introduction of React still inflicts a significant operational overhead, I would say we should stay with the current jquery based approach, just moving from the Go templating to assembling the UI based on the API response.

@drumilpatel2000 what do you think? Does that make sense?

@juliusv
Copy link
Member

juliusv commented Feb 17, 2020

Yeah, I'm aware of the bit of extra functionality, but it's still pretty tiny IMO.

Honestly I would just keep the existing server-side rendering, as you will be able to avoid so much complexity and thousands of lines of extra code by just keeping what you have. This PR here only adds the top NavBar so far, but doesn't have any of the content yet, which would have to be asynchronously fetched and displayed, with error handling, interpretation of the returned JSON, and all that shebang. It's worth it for Prometheus and Alertmanager, but it just seems like too much code and complexity overhead for the Pushgateway to me.

Right now the footprint is pretty small:

You'd likely 10x that by introducing React.

@beorn7
Copy link
Member

beorn7 commented Feb 17, 2020

But what do you think about utilizing the API to populate the DOM (via plain jquery or similar, not react) and avoiding the Go server-side templating?

@weastel
Copy link
Contributor Author

weastel commented Feb 17, 2020

I completely agree with @juliusv of React overkilling the purpose of pushgateway.

But what do you think about utilizing the API to populate the DOM (via plain jquery or similar, not react) and avoiding the Go server-side templating?

According to me, it seems reasonable to go through the API way than rendering using go templates as It will reductantly does the same thing.

@juliusv
Copy link
Member

juliusv commented Feb 17, 2020

But what do you think about utilizing the API to populate the DOM (via plain jquery or similar, not react) and avoiding the Go server-side templating?

Yeah sorry, my reply kind of mixed React and using the API to populate the DOM because I see them correlating strongly. I.e. if you wanted to use the API to populate the DOM, you'll already need way more moving pieces on the client-side (JS-based template engine, data fetching, error handling, state handling, etc.), so that React would be helpful there over doing it all "manually" with jQuery and Handlebars templates and such. So I think I'd recommend just staying with the server-side rendering for now unless there's a planned complexity increase of the UI overall.

@beorn7
Copy link
Member

beorn7 commented Feb 17, 2020

It seems we all agree React is overkill here. But if I understand you two correctly, @juliusv recommends to leave the Go templating in place, while @drumilpatel2000 thinks rendering the UI from the API response in (whatever flavor of) Javascript seems feasible.

@boyskila
Copy link

PGW looks too small to worth to be converted to react app IMHO. From the other side once the whole setup is done is very easy to add new things. The question is how much this will grow in the future and how complex the UI will be? If it's just to show a few tables and two dialogs it's really much much simpler to remain with the template

@beorn7
Copy link
Member

beorn7 commented Feb 19, 2020

I'll close this as we all seem to agree that we won't go for React in the PGW UI.

@drumilpatel2000 if you want to give a more lightweight usage of the API for the UI rendering a spin, feel free to do so. For the time being, I'm also happy to keep the current template-based server-side-rendering in place.

@beorn7 beorn7 closed this Feb 19, 2020
@weastel weastel deleted the new_react_ui branch February 19, 2020 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants