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

Add dashboard panels to all available dashboards #34

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lennarkivimae
Copy link

Like the guy in this thread. I also was in need of a way to automatically add analytics panel to all of the dashboards.
The flow is both automatic and also trigger-able with /patch-dashboards endpoint. Manual trigger can be useful for testing.
Automatic flow is behind timer. This checks dashboards after your specified timeout, and adds analytics to new dashboards if present. In team environment, this helps to reduce need for each person to manually add the dashboard, hence making it more surefire way to get statistics on the dashboards in use.

Adding interval to add analytics panel for all of the dashboards. Eases
maintenance in team environment, where dev-s don't need to think about
adding an panel manually. Interval runs once in 24 hours
Restructure input order
Add extra params to increase testability / flexibility
Add default value for timeout, for easier usage
simplify and increase readability
Move filter check outside of updateDashboards func. Making function
signature correct, update might not of happened at all

increase request failed error information amount for better
understanding why the request failed
@MacroPower
Copy link
Owner

Thank you for the PR! This will be really helpful to have. I have been a bit busy but should have time to test & review properly in the next few weeks.

Just taking a quick initial look, my main concern would be adding additional functionality, especially endpoints, without requiring opt-in from users. Ideally I want to maintain compatibility and not have people run into issues if they upgrade to a newer release without changing any of their arguments / env vars. Also, due to the nature of this application, I know there are some folks that are running it on the Internet to collect data from users. Technically I know you'd have to actually add credentials for the new endpoint to have any impact, but I would rather not drop this on people in-place all the same.

A few options there would be:

  1. Adding a new parameter & environment variable like --dashboard-updates=true / ENABLE_DASHBOARD_UPDATES=true.
  2. Adding a subcommand e.g. macropower_analytics_panel_server dashboard update which calls AddAnalyticsToDashboards and then by default just exits 0.

I think option 1 would be a bit easier, especially given that you / end users wouldn't have to create any new container/job/etc. But option 2, using a subcommand, might be better because it could allow people not using this server (and instead using other data collection methods) to easily make use of this.

The only other thing that stuck out to me was naming the package worker, which felt non-descriptive of what this code does. I am not super particular on the exact naming but something like dashboard would be better I think. e.g. function would end up looking like dashboard.AddAnalytics rather than worker.AddAnalyticsToDashboards.

I can also do this when I get around to it if you're too busy. Again, thanks a ton for the contribution, a lot of people have asked for this feature so it will be really nice to finally have something available 🙂

@lennarkivimae
Copy link
Author

Thank you for the PR! This will be really helpful to have. I have been a bit busy but should have time to test & review properly in the next few weeks.

Just taking a quick initial look, my main concern would be adding additional functionality, especially endpoints, without requiring opt-in from users. Ideally I want to maintain compatibility and not have people run into issues if they upgrade to a newer release without changing any of their arguments / env vars. Also, due to the nature of this application, I know there are some folks that are running it on the Internet to collect data from users. Technically I know you'd have to actually add credentials for the new endpoint to have any impact, but I would rather not drop this on people in-place all the same.

A few options there would be:

1. Adding a new parameter & environment variable like `--dashboard-updates=true` / `ENABLE_DASHBOARD_UPDATES=true`.

2. Adding a subcommand e.g. `macropower_analytics_panel_server dashboard update` which calls AddAnalyticsToDashboards and then by default just exits 0.

I think option 1 would be a bit easier, especially given that you / end users wouldn't have to create any new container/job/etc. But option 2, using a subcommand, might be better because it could allow people not using this server (and instead using other data collection methods) to easily make use of this.

The only other thing that stuck out to me was naming the package worker, which felt non-descriptive of what this code does. I am not super particular on the exact naming but something like dashboard would be better I think. e.g. function would end up looking like dashboard.AddAnalytics rather than worker.AddAnalyticsToDashboards.

I can also do this when I get around to it if you're too busy. Again, thanks a ton for the contribution, a lot of people have asked for this feature so it will be really nice to finally have something available 🙂

Thank you for the reply and feedback!
Sorry for such a late response, as have I been bit busy aswell.
In regards to suggestion, I think they're great. I'll try to implement the changes in near future!

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.

2 participants