powa-team / powa-web

PoWA user interface
http://powa.readthedocs.io/
73 stars 31 forks source link

Complete rewite of the browser part (js, css) of the UI #165

Closed pgiraud closed 7 months ago

pgiraud commented 1 year ago

With this PR the previous frameworks:

Are replaced by:

To test this, you have two options (yet to be documented):

I'm looking forward for your feedback.

[1] the built assets are meant to be committed along with releases.

pgiraud commented 1 year ago

I forgot to mention that we tried to keep it iso-functional. This is the first step for possible future changes to the design and layout a bit to improve the usability.

For testing purpose, in order to make sure that the information displayed in the interface is correct, it's possible to run 2 versions of powa-web at the same time. Running with the same repository data.

In https://github.com/powa-team/powa-podman/blob/master/dev/powa-dev-demo.yml one can add the following section:

powa-web-dev-master: 
  image: powateam/powa-web-dev
  container_name: powa-dev-web-master
  command: "python3 powa-web-dev/run_powa.py"
  ports:
    - 8889:8888
  volumes: 
    - '${POWA_WEB_MASTER_GIT}:/usr/local/src/powa-web-dev'
  links:
    - powa-repository
    - remote-primary
    - remote-standby

Clone powa-web project in a new folder (ie. powa-web-master). The idea is to have one powa-web folder positioned on the current PR branch and one powa-web-master positioned on upstream master branch.

Then run containers with the following command:

POWA_WEB_MASTER_GIT=../../powa-web-master POWA_WEB_GIT=../../powa-web POWA_COLLECTOR_GIT=../../powa-collector podman-compose -f powa-dev-demo.yml up
rjuju commented 1 year ago

First, thanks again for working on that subjet.

I don't really understand the PR though, I waited a bit to make sure that no more commits would be added and it looks like way more than simply the rewrite to get rid of some js stack and use a new one instead. I think that keeping the scope of this first change as small as possible would be easier to review.

About the change itself, I tried to look at the new components but honetly I don't even understand what those are supposed to be used for based on their home pages.

Naively, I see that you get rid of foundation (which I think is a good thing), but I was thinking that relying on bootstrap instead would be the logical replacement. As far as I can see it's not the case, and I'm a bit worried about that. Or is bootstrap part / dependency of some of the new components?

In debug mode : execute npm run dev in a terminal. This should launch a VITE service on http://localhost:5173/. Then run powa in dev mode in an other terminal.

this is quite scary. There's no way to have the python server simply pass the js files like it was the case before?

pgiraud commented 1 year ago

First, thanks again for working on that subjet.

I don't really understand the PR though, I waited a bit to make sure that no more commits would be added and it looks like way more than simply the rewrite to get rid of some js stack and use a new one instead. I think that keeping the scope of this first change as small as possible would be easier to review.

I wanted to propose this PR even though that there were some additional small fixes to be made. I could have waited a bit more but I wanted the team to have a look at the new look and feel.

To be honest, I don't think it would have been possible to do get rid of only some js stack, ie. one at a time. The initial goal was to get rid of backbone and foundation.

The choice made :

Then as a consequence, the choice of ViteJS was straightforward. It replaces bower and is reponsible for the building and dependency management.

Rickshaw which was used to display the graphs could have been kept at first even though compatibility hacks would have probably been required. I decided to get rid of it as well and replaced it with d3.

In summary, it didn't seem possible to split this huge change into smaller steps. If this is a blocker I can of course consider splitting into smaller commits.

About the change itself, I tried to look at the new components but honetly I don't even understand what those are supposed to be used for based on their home pages.

The only thing to know is that with VueJS it's now possible to have the component code all in one place (aka. Single File Components). In my opinion, it makes things a lot easier to understand.

Naively, I see that you get rid of foundation (which I think is a good thing), but I was thinking that relying on bootstrap instead would be the logical replacement. As far as I can see it's not the case, and I'm a bit worried about that. Or is bootstrap part / dependency of some of the new components?

In this rewrite, we don't use bootstrap at all. Instead we're using vuetify because it is more popular and has a more complete and up to date set of components available.

In debug mode : execute npm run dev in a terminal. This should launch a VITE service on http://localhost:5173/. Then run powa in dev mode in an other terminal.

this is quite scary. There's no way to have the python server simply pass the js files like it was the case before?

No, I don't think it's possible. Please note that in the previous version, the libraries' code had to be included in the application code. Now they are installed in node modules via npm. It makes future upgrade easier.

rjuju commented 1 year ago

Thanks for the commits that show how upgrading components is done! Could you also add some documentation (maybe a README.dev or similar) that points out the basics, like

And also document what needs to be done to actually host powa-web in a production style way (this one should eventually be documented in https://github.com/powa-team/powa/blob/master/docs/components/powa-web/deployment.rst but having it locally would still help).

pgiraud commented 1 year ago

Thanks for the commits that show how upgrading components is done! Could you also add some documentation (maybe a README.dev or similar) that points out the basics, like

  • how to setup a dev environment from scratch for the powa-web specific part (ie. ignoring the postgres / extension part)
  • how to check for lib updates and how to update them
  • how to run powa-web locally for dev purpose, how to teardown anything that was spawned and how to check that everything that was spawned is stopped (I'm mostly thinking about the extra vite service thing, maybe others)

And also document what needs to be done to actually host powa-web in a production style way (this one should eventually be documented in https://github.com/powa-team/powa/blob/master/docs/components/powa-web/deployment.rst but having it locally would still help).

I added a CONTRIBUTING.md file. Please let me know if it is clear enough and works for you.

rjuju commented 1 year ago

I added a CONTRIBUTING.md file. Please let me know if it is clear enough and works for you.

Thanks a lot, it looks great!

After a quick look at it, I'm quite happy with the requirements and the general workflow, as nodes 16 doesn't look like a crazy recent version, and the various scenario needed for the common usages are straightforward (and way better than what they currently are). I will try to have a more thorough look at the PR shortly but it overall looks great! Again, I want to emphasize how grateful I'm for this work, congrats to all people involved.

About merging that PR, and then all the other improvements you already made on the UI (which are also really welcome), I was thinking integrating that with the upcoming v5 version of powa that I'm already currently working on, which should hopefully be released somewhere around this summer. Would that work for you? As far as I can see there are only a few minor conflicts, which I can take care of if you prefer. If that's ok for you I can build and maintain specific "v5" versions of various podman images so you can easily work on that branch. The powa-archivist part is probably going to change quite often, but trashing it and recreating it regularly shouldn't be too much of a problem, especially since most of the changes that would impact the UI are already done.

The reason for that is that this rewrite is a huge improvement. It will probably change quite a lot the general UI/UX so it would be weird to drop that in a minor version release. Also, I'm imagining that you may need a bit of time to adjust some details and/or add other changes, and since we only maintain a single major version it will give additional time for that while still having the possibility to release fixes for the current v4.

pgiraud commented 1 year ago

I added a CONTRIBUTING.md file. Please let me know if it is clear enough and works for you.

Thanks a lot, it looks great!

After a quick look at it, I'm quite happy with the requirements and the general workflow, as nodes 16 doesn't look like a crazy recent version, and the various scenario needed for the common usages are straightforward (and way better than what they currently are). I will try to have a more thorough look at the PR shortly but it overall looks great! Again, I want to emphasize how grateful I'm for this work, congrats to all people involved.

Thanks.

About merging that PR, and then all the other improvements you already made on the UI (which are also really welcome), I was thinking integrating that with the upcoming v5 version of powa that I'm already currently working on, which should hopefully be released somewhere around this summer. Would that work for you? As far as I can see there are only a few minor conflicts, which I can take care of if you prefer. If that's ok for you I can build and maintain specific "v5" versions of various podman images so you can easily work on that branch. The powa-archivist part is probably going to change quite often, but trashing it and recreating it regularly shouldn't be too much of a problem, especially since most of the changes that would impact the UI are already done.

Should I try rebasing the current branch onto your v5 branch in anticipation? Having v5 versions of podman images would make things a lot easier. Thanks.

The reason for that is that this rewrite is a huge improvement. It will probably change quite a lot the general UI/UX so it would be weird to drop that in a minor version release. Also, I'm imagining that you may need a bit of time to adjust some details and/or add other changes, and since we only maintain a single major version it will give additional time for that while still having the possibility to release fixes for the current v4.

The earlier we can work on the UI rewrite on top of v5, the better.

rjuju commented 1 year ago

Should I try rebasing the current branch onto your v5 branch in anticipation?

Assuming you're ok with the plan I mentioned sure!

Having v5 versions of podman images would make things a lot easier. Thanks.

I just pushed a new powateam/powa-archivist-git:v5 image. I think that's the only specific image you need to work on the v5, as you need a local copy of powa-web (on this branch rebased over v5) and powa-collector (on v5 branch). Let me know if it's working as intended.

The earlier we can work on the UI rewrite on top of v5, the better.

Agreed.

pgiraud commented 1 year ago

I tried running powa in dev mode with podman, but I got the following errors (found in container logs) in the 3 container running "powa-archivist:v5" image:

exec container process `/usr/local/bin/docker-entrypoint.sh`: Exec format error

FYI, I'm running the following command (from powa-podman/dev directory):

POWA_WEB_GIT=../../powa-web POWA_COLLECTOR_GIT=../../powa-collector podman-compose -f powa-dev-demo.yml up
rjuju commented 1 year ago

Ah, sorry I built the image on an arm machine and that's incompatible with an amd64 machine. I just pushed a new version, can you check if that solves the problem?

pgiraud commented 1 year ago

Thanks.

The current branch is now rebase on v5. It seems to work well.

rjuju commented 1 year ago

Great news! I will try to review the PR and test it thoroughly. We apparently hit different code path, and my local test setup heavily relies on the newest stuff changed in v5 so hopefully we should get a somewhat full manual testing of the UI between you and me.

rjuju commented 1 year ago

I think you used a wrong branch or something during the rebase. The CONTRIBUTING.md isn't there anymore, and the extra commits to bump the dependencies are also missing.

pgiraud commented 1 year ago

Indeed. Fixed.

rjuju commented 1 year ago

Thanks. First initial feedback:


> powa@0.1.0 lint
> eslint --ext .js,.vue --fix powa/static/

Oops! Something went wrong! :(

ESLint: 8.25.0

ESLint couldn't find the config "../.eslintrc.json" to extend from. Please check that the name of the config is correct.

The config "../.eslintrc.json" was referenced from the config file in "/home/rjuju/git/powa/powa-web/powa/static/bower_components/bootstrap/build/.eslintrc.json".

If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.

husky - pre-commit hook exited with code 2 (error)

Maybe you forgot to git-add some file in the tree?

I locally removed the pre-commit to be able to commit some fix, but I'm a bit worried about an undocumented pre-commit hook, especially since it can fail on something different that what it seems to be checking.

<!-"... is not valid JSON at JSON.parse () at store.js:34:27 (anonymous) @ store.js:34 Promise.then (async) (anonymous) @ store.js:33 (anonymous) @ lodash.js:4967 baseForOwn @ lodash.js:3032 (anonymous) @ lodash.js:4936 forEach @ lodash.js:9410 loadData @ store.js:31 (anonymous) @ main.js:107 VM11630:1 Uncaught (in promise) SyntaxError: Unexpected token '<', "

<"... is not valid JSON at JSON.parse () at store.js:34:27 (anonymous) @ store.js:34 Promise.then (async) (anonymous) @ store.js:33 (anonymous) @ lodash.js:4967 baseForOwn @ lodash.js:3032 (anonymous) @ lodash.js:4936 forEach @ lodash.js:9410 loadData @ store.js:31 (anonymous) @ main.js:107 text.js:7 GET http://127.0.0.1:8888/server/0/metrics/database/powa/query/5057148677207632030/wait_events/?from=2023-04-25%2016%3A11%3A45%2B0800&to=2023-04-25%2017%3A11%3A45%2B0800 501 (Not Implemented) text_default3 @ text.js:7 (anonymous) @ store.js:32 (anonymous) @ lodash.js:4967 baseForOwn @ lodash.js:3032 (anonymous) @ lodash.js:4936 forEach @ lodash.js:9410 loadData @ store.js:31 (anonymous) @ main.js:107 text.js:2 Uncaught (in promise) Error: 501 Not Implemented at responseText (text.js:2:27) responseText @ text.js:2 Promise.then (async) (anonymous) @ store.js:33 (anonymous) @ lodash.js:4967 baseForOwn @ lodash.js:3032 (anonymous) @ lodash.js:4936 forEach @ lodash.js:9410 loadData @ store.js:31 (anonymous) @ main.js:107 3content.js:7 Uncaught TypeError: Cannot read properties of undefined (reading '0') at Array. (content.js:7:88534) at c.trigger (content.js:7:81786) at HTMLDocument. (content.js:7:81496) (anonymous) @ content.js:7 c.trigger @ content.js:7 (anonymous) @ content.js:7

rjuju commented 1 year ago

Thanks for the work on the changes marker. FTR one feature missing in the rewrite is when you click on the marker. In the older version, it makes the hover tooltip stay active and add a vertical line above (now should be below) the marker so you can easily eyeball the graphs before/after the change. Is that something that can be done with the rewrite?

Note that one annoying thing in the current version is that the same marker for different graphs are independent. Ideally, we'd want to have to hover sticky thing and the vertical line be synced across all graphs.

aadrian commented 1 year ago

@rjuju any chance merging this?

Even if there are still outstanding issues, it would be much easier to use it as a "base" for further fixings.

Thank you.

rjuju commented 1 year ago

@aadrian it might not be self evident from this PR side but we have off-line interactions about this work and things are moving in the right direction and I expect the feature to be ready soon.

I'm not opposed to merging something that still has known issues, but as discussed above this will be released as of powa 5.0.0, which is still a work in progress (I implemented compatibility with relocation of any of the underlying extensions already and I'm now working on a new "per-database snapshot" feature) so it's not really rushed anyway. I'm expecting version 5.0.0 to be released sometime during summer, so keeping it as a distinct branch for now makes it easier to rework stuff without spamming the git history.

aadrian commented 1 year ago

@rjuju

but as discussed above this will be released as of powa 5.0.0,

I think major version bump is even expected, since it's such a big rewrite.

so keeping it as a distinct branch for now makes it easier to rework stuff without spamming the git history.

This is a PR however, not really a "release branch" to be used by the community as a base for further PRs.

The simplest to handle for the community would be have a release_4.x where only "fixings" would be accepted, and do the further development on master , or have release_5.x branch (if for version 4 still major feature updates are to be expected).

rjuju commented 1 year ago

We only maintain a single major version, which is the master branch. So for now master is pointing as version 4.2.0 (which is under development as we keep developping it for less massive changes, and should be released in a few days). All the developments for version 5.0.0 is done in different "v5" branches in multiple projects:

This PR is still opened again master but should actually be targetting the v5 branch, as it will only be merged there. So whether this PR is merged now, in a month or in 3 months won't have any impact on the upcoming 4.2.x versions, or even 4.x versions is we release more major versions before the v5 is ready, and people should still base their changes against master. In the mean time I take care of rebasing all the v5 branches against master (which is a lot of work), and @pgiraud takes care of rebasing this branch against the rebased v5 (which I believe barely have any conflicts so it's mostly free).

pgiraud commented 1 year ago

Thanks. First initial feedback:

  • first of all, it seems to mostly work quite great :) It's also globally way better looking (the old interface is honestly quite ugly), although this new versions feels a bit like austerity. Maybe that's because of the graphs (see below), or me just being too used to the old version. Not a blocker for now, especially since it's quite better already.

I think it's a first step. But we could probably change the theme a bit. It may a also be a matter of taste. I plan to work on the interface a bit more after the release of v5. Rearrange things for example to make things more readable.

Also, @pirlgon gave a first try at adding a dark/light switcher.

  • the interface if the vite service isn't running, or not reachable yet (or I'm assuming if js is deactivated in the browser or the httpd configuration is broken or...) is quite bad. [...]

Done.

  • the .husky/pre-commit seems broken. [...]

In your case, for an unknown reason pre-commit is having problems with a file that isn't in the repository anymore (powa/static/bower_components/bootstrap/build/.eslintrc.json). Can you please check if this file exists in your directory? It shouldn't be the case.

  • the graphs used to have some background grid to help reading them. Could they be added back? A totally blank graph looks weird to me, and I find it harder to follow the time without it

Fixed.

  • the config changes red triangles looks a bit scary. Maybe use a different color at least? Note also that they stick to the lower axs which makes the graphs harder to read if you have a lot of them. I kept restarting my db server to fix some unrelated bug, and I have a bunch of those, which can also hide the very small time ticks on the axis

Changed. I hope you like it now.

  • the new time selector is nice, but I feel like there are too many configurations. The first one(s) seems a bit unnecessary as the default frequency is 5 min (and it's somewhat recommended to not go have too frequent snapshots). Maybe removing the 5 and 15 min choices would be better. Also, AFAICS the date picker only allows you to pick a date. Isn't there a component available where you could select the time too?

Done.

  • the links in the grids are not "real link" anymore but only a js action. [...]

Fixed.

  • the "Most executed values" graphs (in /server/X/database/bench/query/Y/qual/Z) seems to be broken

Fixed with a "distribution" grid.

  • there are a lot of error in the js console.

Fixed.

rjuju commented 1 year ago

I think it's a first step. But we could probably change the theme a bit. It may a also be a matter of taste. I plan to work on the interface a bit more after the release of v5. Rearrange things for example to make things more readable.

Ok. This can definitely done outside of this PR.

In your case, for an unknown reason pre-commit is having problems with a file that isn't in the repository anymore (powa/static/bower_components/bootstrap/build/.eslintrc.json). Can you please check if this file exists in your directory? It shouldn't be the case.

I indeed have such a file. That file isn't tracked, and I can't really get rid of the bower_component just yet as I still need to switch back and forth with the v5 branch for the other things I'm working on. I'm assuming that this won't be a problem anymore once the PR is merged and bower_component/ will be a distant memory?

Changed. I hope you like it now.

Yes it's way better, thanks a lot! One minor gripe though is that it's above the graph and the hover popup expand on the bottom, it hides a bit the graph. It could become problematic if you have multiple changes at the same timestamp, or when we will eventually add back the feature to make the hover popup and vertical line "stick" when we click on the marker.

Thanks a lot for all the other changes, that's perfect!

I also just noticed that you now show all the metrics on hover for a given timestamp, and also group and split the legends on each borders depending on whether they're for Y or Y2 axis. That's insanely cool, thanks a lot!

One minor gripe is that the hoover popup that shows all the values doesn't seem to use the same pattern for units as the graph legend in some cases. I don't know if it's only for the Y2 axis or only for axis without a unit, but for instance on the server page the "Query runtime per second" (server/X/metrics/databases_globals/ datasource) the "Queries per sec" on the axis is displayed nicely ("0 .. 2k .. 4k .. 6k .. 8k"), but on hoover it uses a scientific notation (e.g. 6.7e+3). That's not critical (especially since we don't have a lot of graphs without units), but a more friendly notation could help and I'm assuming it's trivial to fix. If it's not, or if you just want to postpone it for later I can create a new issue for that instead.

I don't see any other problem for now, although I didn't have time to test it in "production mode" rather than debug mode. BTW, how does things work in that mode? Are all the necessary assets contained in the repository in minified/uglified/whateveried version so that you simply have to run powa-web.py, or is there any additional steps needed? In the original message you mentioned that you need to run npm run build to have the assets ready, but I don't think anyone will want to either install npm on their production server or recompile the assets everytime they update a package (assuming they both know about it and don't forget to). Running it locally I see it generates a  ~2MB directory with 10 files. Could we just recompile, commit and bundle those files before each release so it's uploaded to pypi (and the rhel/deb repositories) and users can just start powa-web?

rjuju commented 1 year ago

Also I just force-pushed the v5 branch with some minor unrelated changes, and also a new entry in the changelog for the upcoming v5. Could you take care of adding the necessary entry(ies) in the v5 part of the changelog for this work, crediting the needed persons?

pgiraud commented 1 year ago

In your case, for an unknown reason pre-commit is having problems with a file that isn't in the repository anymore (powa/static/bower_components/bootstrap/build/.eslintrc.json). Can you please check if this file exists in your directory? It shouldn't be the case.

I indeed have such a file. That file isn't tracked, and I can't really get rid of the bower_component just yet as I still need to switch back and forth with the v5 branch for the other things I'm working on. I'm assuming that this won't be a problem anymore once the PR is merged and bower_component/ will be a distant memory?

It actually IS tracked. https://github.com/powa-team/powa-web/tree/master/powa/static/bower_components So switching from one branch to another shouldn't be a problem.

One minor gripe is that the hoover popup that shows all the values doesn't seem to use the same pattern for units as the graph legend in some cases. I don't know if it's only for the Y2 axis or only for axis without a unit, but for instance on the server page the "Query runtime per second" (server/X/metrics/databases_globals/ datasource) the "Queries per sec" on the axis is displayed nicely ("0 .. 2k .. 4k .. 6k .. 8k"), but on hoover it uses a scientific notation (e.g. 6.7e+3). That's not critical (especially since we don't have a lot of graphs without units), but a more friendly notation could help and I'm assuming it's trivial to fix. If it's not, or if you just want to postpone it for later I can create a new issue for that instead.

Now fixed.

I don't see any other problem for now, although I didn't have time to test it in "production mode" rather than debug mode. BTW, how does things work in that mode? Are all the necessary assets contained in the repository in minified/uglified/whateveried version so that you simply have to run powa-web.py, or is there any additional steps needed? In the original message you mentioned that you need to run npm run build to have the assets ready, but I don't think anyone will want to either install npm on their production server or recompile the assets everytime they update a package (assuming they both know about it and don't forget to). Running it locally I see it generates a  ~2MB directory with 10 files. Could we just recompile, commit and bundle those files before each release so it's uploaded to pypi (and the rhel/deb repositories) and users can just start powa-web?

When running npm run build, powa/static/dist should contain all the assets required for PoWA web to work in production mode. We could bundle everything in one (big) single JS file but splitting into chunks makes more sense to me. Unless we upgrade libraries versions most of the chunks won't change in upcoming releases.

Please note that most of the chunks size is rather small. The only large one is for grafana. I'm myself surprised since the amount of Grafana's code we rely on is very small. I unfortunately didn't take time to analyze the possible problem.

My idea is that for each release the maintainer runs the build command and commits the changes found in powa/static/js/dist. Then all the files in dist (including the manifest.json file) should be shipped to pypi (including the manifest.json file).

pgiraud commented 1 year ago

Also I just force-pushed the v5 branch with some minor unrelated changes, and also a new entry in the changelog for the upcoming v5. Could you take care of adding the necessary entry(ies) in the v5 part of the changelog for this work, crediting the needed persons?

Done.

rjuju commented 1 year ago

It actually IS tracked. https://github.com/powa-team/powa-web/tree/master/powa/static/bower_components So switching from one branch to another shouldn't be a problem.

That directory is tracked, not that file or the bootstrap directory, so it stays when I change the branch as long as it's not conflicting. I'm not sure how it ended up there, I will be happy to trash it as soon as we merge this branch and v5 in master.

Now fixed.

Thanks, that's perfect!

We could bundle everything in one (big) single JS file but splitting into chunks makes more sense to me. Unless we upgrade libraries versions most of the chunks won't change in upcoming releases.

Agreed, there's no point in doing that and it should help containing the git repo size.

Please note that most of the chunks size is rather small. The only large one is for grafana. I'm myself surprised since the amount of Grafana's code we rely on is very small. I unfortunately didn't take time to analyze the possible problem.

That's not a problem for me.

My idea is that for each release the maintainer runs the build command and commits the changes found in powa/static/js/dist. Then all the files in dist (including the manifest.json file) should be shipped to pypi (including the manifest.json file).

Yes that's what I was thinking. I will try to come up with a RELEASING.md doc for the future 5.0.0 version.

Done.

Thanks!

rjuju commented 1 year ago

I tried to look a bit at the timeline annotation code. I don't really know the underlying framework but if I understand correctly this is for now coded in a way that only a single event can be displayed at the same time. That seems fundamentally incompatible with either making those events "sticky" on click or synchronizing the "show on hover" on all graphs that I mentioned at https://github.com/powa-team/powa-web/pull/165#issuecomment-1537296485. Is it something that you plan to work on? If yes, would that be as part of the PR or later on?

rjuju commented 1 year ago

I also just realized that the grids are missing the "export to CSV" feature.

pgiraud commented 1 year ago

I tried to look a bit at the timeline annotation code. I don't really know the underlying framework but if I understand correctly this is for now coded in a way that only a single event can be displayed at the same time. That seems fundamentally incompatible with either making those events "sticky" on click or synchronizing the "show on hover" on all graphs that I mentioned at #165 (comment). Is it something that you plan to work on? If yes, would that be as part of the PR or later on?

I would like to work on this part but I think "later on" would be a good option. Unless many users are really using the on-click sticky event feature.

rjuju commented 1 year ago

I would like to work on this part but I think "later on" would be a good option. Unless many users are really using the on-click sticky event feature.

I don't know how many users rely on that, but it doesn't seem critical to have it immediately. I'm fine with postponing that to a later version.

rjuju commented 1 year ago

Looking at the grids on configuration pages, the old version distinguishes true (green tick) from false (red cross) from NULL (prohibited sign), while the rewrite apparently uses a red cross for both false and NULL. We need to distinguish both situation, as for instance an extension not being available is not the same as not knowing whether an extension is available or not. Could you put back some similar icon when the value is NULL?

Another minor problem: the old version would display a drop down in the breadcrumbs to quickly choose the server or the database, rather than having to scroll down on the page and find the one you're looking for. Is that something you could bring back? Not that while it's nice to have this is not very critical or rushed.

rjuju commented 1 year ago

I forgot to mention that the fixup commits look good to me, they can get fixuped in the original commit any time you want.

pgiraud commented 1 year ago

Looking at the grids on configuration pages, the old version distinguishes true (green tick) from false (red cross) from NULL (prohibited sign), while the rewrite apparently uses a red cross for both false and NULL. We need to distinguish both situation, as for instance an extension not being available is not the same as not knowing whether an extension is available or not. Could you put back some similar icon when the value is NULL?

Fixed.

Another minor problem: the old version would display a drop down in the breadcrumbs to quickly choose the server or the database, rather than having to scroll down on the page and find the one you're looking for. Is that something you could bring back? Not that while it's nice to have this is not very critical or rushed.

I'm aware of that. I struggled reimplementing this feature. I haven't given up yet but I don't think I'll manage to get it back easily soon.

rjuju commented 1 year ago

missing prohibited sign Fixed.

Thanks! Although it seems that you chose U+1F6C7, which seems like a not so ideal codepoint, as it's described as:

This Unicode character has no emoji version, meaning this is intended to display only as a black and white glyph on most platforms. It has not been Recommended For General Interchange (RGI) — as an emoji — by Unicode.

On my current platform I only see a black rectangle, I guess this is a recent enough codepoint that I need a specific font. Maybe some other character would be more suitable, e.g. U+20E0 ( ⃠ ) or U+2205 ( ∅ ), would be a better choice.

I'm aware of that. I struggled reimplementing this feature. I haven't given up yet but I don't think I'll manage to get it back easily soon.

No worries. It's not a blocker as the link is available from other places anyway.

pgiraud commented 1 year ago

On my current platform I only see a black rectangle, I guess this is a recent enough codepoint that I need a specific font. Maybe some other character would be more suitable, e.g. U+20E0 ( ⃠ ) or U+2205 ( ∅ ), would be a better choice.

I changed the character. Can you please that it works on your plateform.

rjuju commented 1 year ago

Thanks, this one works on all my machines!

rjuju commented 12 months ago

I just noticed that the wizard component doesn't correctly handle "initial errors".

You can easily reproduce the problem with a remote server that doesn't have pg_qualstats installed, or just trying to display a non existing database (e.g. http://127.0.0.1:8888/server/1/database/blablabla/overview/)

The "Optimize this database !" button is still visible.

pgiraud commented 12 months ago

I just noticed that the wizard component doesn't correctly handle "initial errors".

You can easily reproduce the problem with a remote server that doesn't have pg_qualstats installed, or just trying to display a non existing database (e.g. http://127.0.0.1:8888/server/1/database/blablabla/overview/)

The "Optimize this database !" button is still visible.

The branch now includes a fixup commit to fix this.

pgiraud commented 11 months ago

Converting to draft because of a bug in recent commits . See https://github.com/pgiraud/powa-web/pull/59#issuecomment-1628853044.

pgiraud commented 10 months ago

I just pushed a new version of the branch. It includes commits that prevent the page from reloading when navigating. I think it's a nice improvement for the user experience. It gives an overall impression of speed and fluidity.

Most of the UI templates are now in VueJS files. There's a better separation between server templates and UI ones.

I'm pretty happy with the result.

rjuju commented 10 months ago

I just pushed a new version of the branch. It includes commits that prevent the page from reloading when navigating. I think it's a nice improvement for the user experience. It gives an overall impression of speed and fluidity.

Do you mean avoid reloading the whole HTML DOM when switching from per-server to per-database pages and so on?

I agree that it's a nice improvement that makes things smoother! Thanks a lot.

One complaint though: while this seem to work well with regards to the browser history (you can still go to back and forth as expected), the breadcrumb and the page title aren't updated as expected. It therefore makes the history less useful (and all you can see is e.g. "All databases" rather than where it actually was), and with the "frozen" breadcrumbs it becomes really confusing. Any chance to fix that?

pgiraud commented 10 months ago

One complaint though: while this seem to work well with regards to the browser history (you can still go to back and forth as expected), the breadcrumb and the page title aren't updated as expected. It therefore makes the history less useful (and all you can see is e.g. "All databases" rather than where it actually was), and with the "frozen" breadcrumbs it becomes really confusing. Any chance to fix that?

Good catch. This was indeed that I forgot to take into account. Now fixed.

One other issue I need to handle is the alert messages sometimes being showed when they shouldn't. For example, as of now, navigating to the configuration page for one server, a message "Collector status for this instance: running" will show. And then, this message shows up on any upcoming navigation change. I'll work on this later today.

pgiraud commented 9 months ago

The problem on alert message being displayed untimely is fixed.

rjuju commented 7 months ago

@pgiraud I pushed the v5 branch rebased over the recent 4.2.1 version, with also some modifications to deal with the new counters added in pg17 recently. Could you update your local branch? If not I can take care of it if I'm allowed to push on it.

Note that I plan on merging this branch as soon as I can once done before starting some new work in different other area of powa.

pgiraud commented 7 months ago

I just force pushed an update.

I just had a quick try. I didn't see anything wrong except an error in the JS console when using the Optimize wizard.

"Error: Parse error: Unexpected "• WHERE co" at line 1 column 1"
rjuju commented 7 months ago

Thanks, I will look into that.

FTR when updating the dependencies, I saw the following:

$ npm ci
[...]
6 vulnerabilities (5 moderate, 1 high)
[...]

$ npm audit
@antfu/utils  <0.7.3
Severity: moderate
antfu/utils vulnerable to prototype pollution - https://github.com/advisories/GHSA-p2fh-2h23-6grg
fix available via `npm audit fix`
node_modules/@antfu/utils
  unplugin-vue-components  0.15.4 - 0.22.9
  Depends on vulnerable versions of @antfu/utils
  node_modules/unplugin-vue-components

postcss  <8.4.31
Severity: moderate
PostCSS line return parsing error - https://github.com/advisories/GHSA-7fh5-64p2-3v2j
fix available via `npm audit fix`
node_modules/postcss

semver  7.0.0 - 7.5.1
Severity: moderate
semver vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-c2qf-rxjj-qqgw
fix available via `npm audit fix`
node_modules/semver

vite  3.0.2 - 3.2.6
Severity: high
Vite Server Options (server.fs.deny) can be bypassed using double forward-slash (//) - https://github.com/advisories/GHSA-353f-5xf4-qw67
fix available via `npm audit fix`
node_modules/vite

word-wrap  <1.2.4
Severity: moderate
word-wrap vulnerable to Regular Expression Denial of Service - https://github.com/advisories/GHSA-j8xg-fqg3-53r7
fix available via `npm audit fix`
node_modules/word-wrap

6 vulnerabilities (5 moderate, 1 high)
rjuju commented 7 months ago

There's also a problem on the checkpointer graphs. The console shows those errors:

[Vue warn]: Error in render: "TypeError: Cannot read properties of undefined (reading 'metrics')"

found in

---> <Graph> at /Users/rjuju/git/powa/powa-web/powa/static/js/components/dynamic/Graph.vue
       <Dashboard> at /Users/rjuju/git/powa/powa-web/powa/static/js/components/dynamic/Dashboard.vue
         <VCard>
           <VTabItem>
             <VTabsItems>
               <VCard>
                 <Tabcontainer> at /Users/rjuju/git/powa/powa-web/powa/static/js/components/dynamic/Tabcontainer.vue
                   <Dashboard> at /Users/rjuju/git/powa/powa-web/powa/static/js/components/dynamic/Dashboard.vue
                     <VMain>
                       <VApp>
                         <App> at /Users/rjuju/git/powa/powa-web/powa/static/js/App.vue
                           <Root>
TypeError: Cannot read properties of undefined (reading 'metrics')
    at Object.getLabel (Graph.vue:637:1)
    at Graph.vue:792:1
    at Proxy.renderList (vue.runtime.esm.js:2027:22)
    at Proxy.render (Graph.vue:792:1)
    at Vue2._render (vue.runtime.esm.js:2680:28)
    at VueComponent.updateComponent (vue.runtime.esm.js:3870:27)
    at Watcher2.get (vue.runtime.esm.js:3441:33)
    at Watcher2.run (vue.runtime.esm.js:3517:30)
    at flushSchedulerQueue (vue.runtime.esm.js:4116:17)
    at Array.<anonymous> (vue.runtime.esm.js:3139:20)

I unfortunately have no idea what those mean or how to debug it.

It's also missing the latest commit on v5 branch (156dfd4f0770c207b8629b55135cae181d50090c).

rjuju commented 7 months ago

just had a quick try. I didn't see anything wrong except an error in the JS console when using the Optimize wizard. "Error: Parse error: Unexpected "• WHERE co" at line 1 column 1"

According to the stack trace it looks like it's coming from the sql-formatter package?

pgiraud commented 7 months ago

just had a quick try. I didn't see anything wrong except an error in the JS console when using the Optimize wizard. "Error: Parse error: Unexpected "• WHERE co" at line 1 column 1"

According to the stack trace it looks like it's coming from the sql-formatter package?

Fixed.

pgiraud commented 7 months ago

I just added a commit to warn users when trying to show metrics from different sources in the same chart. This is not supported yet. Let's implement this when we need it.