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

fix: add routes data for panel api endpoints #263

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

sea-grass
Copy link
Contributor

When visiting the admin panel after running wws --enable-panel, we are presented with a loading spinner and the worker info never loads. With RUST_LOG="actix_web=debug", we can see the following error when we access the panel at https://127.0.0.1:8080/_panel:

[2024-02-06T00:23:37Z DEBUG actix_web::data] Failed to extract `Data<wws_router::Routes>` for `handle_api_workers` handler. For the Data extractor to work correctly, wrap the data with `Data::new()` and pass it to `App::app_data()`. Ensure that types align in both the set and retrieve calls.

This PR adds Routes through the app's app_data for the api handler. I don't typically work in Rust, so let me know if there's a better/more preferred approach to solve this problem!

Note: The routes are available as part of the AppData that's already exposed to the Actix app, but adding wws-server as a dependency to wws-api-manage so we can reference the AppData type results in a circular dependency.

@vmwclabot
Copy link

@sea-grass, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@vmwclabot
Copy link

@sea-grass, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

@vmwclabot
Copy link

@sea-grass, VMware has approved your signed contributor license agreement.

@ereslibre ereslibre added the 🐛 bug Something isn't working label Feb 22, 2024
Copy link
Contributor

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

Thank you @sea-grass. I can confirm the issue, and that this PR fixes the problem.

I also think it is a great way to approach this problem. The circular dependency problem could be solved by using a different crate and make that one being depended upon by the other two, but I think this is good enough!

@Angelmmiguel
Copy link
Contributor

Hey @sea-grass !

Thank you for the fix here. In the future we plan to add dynamic routing, so this may require a refactor later on but for now is a great solution.

I already triggered the CI and will merge after it! Thank you for your contribution 😄

@Angelmmiguel
Copy link
Contributor

@sea-grass there is an issue that comes from the main branch, not related to your PR. I'm gonna fix it on main so you can rebase before merging it.

@Angelmmiguel
Copy link
Contributor

@sea-grass I can confirm the deny error is already fixed by cac427d, so don't worry about rebasing this branch.

@Angelmmiguel Angelmmiguel merged commit 4b05de2 into vmware-labs:main Feb 23, 2024
5 of 6 checks passed
@Angelmmiguel
Copy link
Contributor

Btw @sea-grass,

I want to take the opportunity to ask you about the admin panel. It is an experimental feature and we have ideas to expand it in the future. What would you expect from this admin panel? Any specific data / action you would like to have there?

Thank you!

@sea-grass sea-grass deleted the fix-admin-panel branch February 23, 2024 15:51
@sea-grass
Copy link
Contributor Author

What would you expect from this admin panel? Any specific data / action you would like to have there?

Hmm that's a good question! I'm not using WWS in any production capacity yet so I'll be speculating a bit, but when I think admin panel I think metrics related to application performance/observability and some configurability regarding behaviour. Something like:

  • A graph showing # of "success responses" (like 200) vs. # of failure responses (like 500), either in aggregate or on a per-worker basis. I think to increase adoption these things could be automatically sent to some metrics sink to be consolidated with the rest of an organization's data but a simple in-memory graph in the WWS admin panel would be a good first step.
  • WWS runtime metrics like CPU/RAM usage.
  • The ability to enable/disable certain workers. Again, not using this in a production capacity so this is purely speculative, but I could imagine a scenario in which an organization would want to temporarily disable a worker that's causing issues without bringing the rest of the workers down. Although, I'm not sure how useful this would be at this time. Say a worker has a bug so you want to disable it, fix the bug, then redeploy a new version. The ability to disable a single worker would be nice so that you wouldn't have to interrupt any other workers, but once you fix the bug, you'll have to restart WWS to load the new version anyway. So, I don't see this being that useful in practice until dynamic/runtime discovery of routes is implemented.

@Angelmmiguel
Copy link
Contributor

What would you expect from this admin panel? Any specific data / action you would like to have there?

Hmm that's a good question! I'm not using WWS in any production capacity yet so I'll be speculating a bit, but when I think admin panel I think metrics related to application performance/observability and some configurability regarding behaviour. Something like:

  • A graph showing # of "success responses" (like 200) vs. # of failure responses (like 500), either in aggregate or on a per-worker basis. I think to increase adoption these things could be automatically sent to some metrics sink to be consolidated with the rest of an organization's data but a simple in-memory graph in the WWS admin panel would be a good first step.
  • WWS runtime metrics like CPU/RAM usage.
  • The ability to enable/disable certain workers. Again, not using this in a production capacity so this is purely speculative, but I could imagine a scenario in which an organization would want to temporarily disable a worker that's causing issues without bringing the rest of the workers down. Although, I'm not sure how useful this would be at this time. Say a worker has a bug so you want to disable it, fix the bug, then redeploy a new version. The ability to disable a single worker would be nice so that you wouldn't have to interrupt any other workers, but once you fix the bug, you'll have to restart WWS to load the new version anyway. So, I don't see this being that useful in practice until dynamic/runtime discovery of routes is implemented.

Thank you for taking the time to share this feedback! This is really valuable as we already discussed in the past some of the features you mentioned, like response metrics. Regarding enable/disable workers, it's an interesting feature when working on an environment with many workers or shares the same instance between different projects 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants