Skip to content

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.

Lennar Kivimäe added 10 commits August 13, 2024 07:45
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!

rename worker package to dashboard, break up api to httpcli package.
Simplifies understanding and breaks up different functionalities
Only enable analytics adder, if grafana url and service account token is
present
Refactor adding analytics logic, fixing a bug that panels would be moved
out of their respective folders
Rename bunch of methods to be more concise, due to package rename
@lennarkivimae
Copy link
Author

I managed to break my promise by a lot unfortunately. On a plus side, I managed to get around to it now. Better late than never. Sorry for this, it was not my intention.

I think I managed to address your concerns.
The feature is only toggled on when grafana-url and dashboard-update-token are provided, as they both are needed for the feature. I currently did not see the need to add special enable-dashboard-updates flag. Currently no one should have those flags passed, hence the feature is disabled by default. This should satisfy the option 1. Unless of course you'd still like that special flag to be in place.
The option 2 I did not get around to do due to time constraints. If this is desired by people in the future. I'm happy to take a look at it then.

As requested, I broke up worker package. I also broke api.go into dashboard.go and http.go. There are now dashboard package and httpcli package. Should be more clear of their intents. httpcli just provides a simple HTTP request helpers. All dashboard update logic is done in dashboard package.

Also added checks if update is even required, and fixed a bug where updating dashboard moves the dashboard out of its folder.

Whenever you have a change, have a look. All feedback and guidance is most appreciated!

Thank you for your time

@lennarkivimae lennarkivimae force-pushed the feature/auto-analytics-adder branch from 4df9966 to ef8d3e0 Compare April 10, 2025 14:22
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