Minor update: remove flux datasource plugin from grafana

BuongiornoTexas commented 4 months ago

Problem Following on from #234, I'd like to propose removing grafana-influxdb-flux-datasource from grafana (and particularly grafana.env).

I'm suggesting this after going through a multi-hour upgrade to version 4.0, where it looks as though most of my problems were down to this plugin failing to install repeatedly (it go there in the end, but it wasn't fun).

Additional context

The plugin is apparently deprecated, with flux support built into grafana from version 7.1 onwards. Given this, I would expect any Powerwall-Dashboard transition to InfluxDB 2.0 will use the built in grafana support rather than the deprecated plugin.
As far as I know, only myself and ThePnuts have made any significant use of flux, and the outcome of my experiment was a clear conclusion that neither I nor my friends should use Flux if at all possible (at least in current state).
I've removed it from my plugins, and grafana has become markedly more responsive.

@ThePnuts do you have a view on this?

ThePnuts commented 4 months ago

I did not have the mentioned issue. When I started with this, I was using the latest version of each service, Grafana, InfluxDB 2.0, Telegraf, etc. and aside from converting to flux, just made minor tweaks to run each part of this independent. So I cant run an upgrade script, I just update each independent docker when I feel like it. Still running latest of each as of today without any issues. For Grafana, I am on version: v10.3.3 (252761264e)

I also have a more "true" setup running concurrently just so I can compare numbers overall vs my flux formula's and what @jasonacox did for influxDB 1.8. I also have this data source reporting on the latest Grafana mentioned above and just had to tweak the depreciated plugins to the current ones.

I do not have an "as instructed", default version of this project installed, so I do not know if removing it would break anything on a standard install, but I assume it wouldn't since its not using flux currently. It would not interfere with anyone that followed my influxdb 2.0/flux instructions since that moves you to independent dockers as mentioned above.

All of that said, I think it should be removed if there is any plan to update Grafana further. If it was me, I would suggest instead to remove/update all depreciated plugins and move to Grafana 10 as it does work currently without any issue. The pie charts look worse though, but that's personal preference. Here is mine using influxdb 1.8 with the updated plugins.

mcbirse commented 4 months ago

I agree that the old "beta" plugin probably should be removed. I didn't even realise it was there to be honest!

I have had a play with Flux previously and had just used the standard InfluxDB data source, and with the dashboard's current version of Grafana & InfluxDB (9.1.2 and 1.8) it works perfectly by simply adding a new data source and changing the query language to Flux. Hence I never noticed that beta plugin.

Certainly seems like no need for the old beta plugin from what I can tell.

We can remove "grafana-influxdb-flux-datasource" from the grafana.env.sample file easily enough... I guess the question follows however, should we also offer to remove this plugin on an existing install? As removing the entry from GF_INSTALL_PLUGINS does not appear to actually remove the plugin if already installed?

This could perhaps be offered to the user when running the upgrade.sh script? Thoughts?

Removal would appear to involve a few steps, i.e. running sed on grafana.env to remove the plugin from the GF_INSTALL_PLUGINS environment string, delete the grafana-influxdb-flux-datasource directory and contents completely from "./grafana/plugins/", and then rebuild with container with docker compose. I think.

@BuongiornoTexas - can you confirm that was how you removed the plugin from your install?

mcbirse commented 4 months ago

As far as I know, only myself and ThePnuts have made any significant use of flux, and the outcome of my experiment was a clear conclusion that neither I nor my friends should use Flux if at all possible (at least in current state).

Oh, I forget to mention, and in reference to the above, something I noticed during my testing/research of Flux recently...

From version 3.0 of InfluxDB, Flux has entered maintenance mode:

https://www.influxdata.com/blog/the-plan-for-influxdb-3-0-open-source/#heading4

So I came to a similar conclusion that perhaps NO ONE should be bothering to invest in Flux.... (and I dumped what I was trying to achieve, in favour of InfluxQL plus a few CQ's instead, which executed 10x faster than my Flux query anyway).

Although it sounds like they will continue to support Flux, it appears to me it could possibly become deprecated?

ThePnuts commented 4 months ago

Oh, I forget to mention, and in reference to the above, something I noticed during my testing/research of Flux recently...

From version 3.0 of InfluxDB, Flux has entered maintenance mode:

https://www.influxdata.com/blog/the-plan-for-influxdb-3-0-open-source/#heading4

So I came to a similar conclusion that perhaps NO ONE should be bothering to invest in Flux.... (and I dumped what I was trying to achieve, in favour of InfluxQL plus a few CQ's instead, which executed 10x faster than my Flux query anyway).

Although it sounds like they will continue to support Flux, it appears to me it could possibly become deprecated?

Oof, I guess its time I think about a move to 3.0 and drop Flux

BuongiornoTexas commented 4 months ago

I agree that the old "beta" plugin probably should be removed. I didn't even realise it was there to be honest!

It has been there a long while - I made brief use of it exploring a query based ToU system.

Removal would appear to involve a few steps, i.e. running sed on grafana.env to remove the plugin from the GF_INSTALL_PLUGINS environment string, delete the grafana-influxdb-flux-datasource directory and contents completely from "./grafana/plugins/", and then rebuild with container with docker compose. I think.

@BuongiornoTexas - can you confirm that was how you removed the plugin from your install?

Yes - this was the process. I also cleaned up half a dozen other plugins that are retired. I suspect that once a plugin is in that directory it is always loaded. And I have definitely got a noticeable performance improvement from removing the redundant plugins.

The mild risk of removing is that people may have upgraded their installations but not their dashboard.json, and so may still be (unknowingly) using outdated plugins. I know I'm lazy about dashboard updates. I probably only do these once or twice a year. I guess upgrade.sh could warn about this?

From version 3.0 of InfluxDB, Flux has entered maintenance mode:

Well that's annoying - I used a simple flux query as part of my pwdusage extension. I guess it is time to go back and revert to the 1.8 InfluxQL API so that it is easy to convert to v3.0 if we ever go that way. Hopefully I will still similar speedups to you with my simple query.

jasonacox commented 4 months ago

Interesting. I looked at the effort required of moving from 1.8 to 2.x and it was significant to port the data and convert the queries to Flux. Their notice about 3.0 indicates there will be a much easier path from 1.8 (or 2.x) to 3.0. Fingers crossed.

@BuongiornoTexas - I'm not completely understanding the issue you opened, but thanks for opening. I assume the consensus is to remove the old Flux plugin.

BuongiornoTexas commented 4 months ago

I made the request to keep it in #234. No longer relevant now, so I was adding it to the list of redundant plugins for removal. @mcbirse pointed that we should also probably actually clean up unused plugins to improve performance (which I'm down with at this point).

And fingers crossed indeed on 3.0.

jasonacox commented 4 months ago

Agree! Thanks @BuongiornoTexas . If any one has time to test and submit a PR, please do. 🙏

BuongiornoTexas commented 4 months ago

Here's a first pass - if it looks good, I'll convert to a PR. Basics are:

Check if there is a difference between the sample and current grafana plugin lists.
If there is, ask the user if they want to update to the current set.
If the user says yes, grafana.env is updated, all existing plugins are deleted, and the grafana image is stopped and deleted.

This should catch the majority of users, and the ones it doesn't are probably doing their own customisation anyway.

I've tested this in a synthetic dashboard folder, and will test this script in my live folder if you are all happy. I'd prefer to avoid doing a full upgrade.sh run given the fun I already have managing a rootless docker install.

#!/bin/bash
# already defined in upgrade.sh.
GF_ENV_FILE="grafana.env"

# Insert this block after line 213 in upgrade.sh

# Create variables as script gets unwieldy otherwise
GF_OLD_PLUGINS=$(sed -n '/GF_INSTALL_PLUGINS=/p' grafana.env)
GF_NEW_PLUGINS=$(sed -n '/GF_INSTALL_PLUGINS=/p' ${GF_ENV_FILE}.sample)

if [ "$GF_OLD_PLUGINS" == "$GF_NEW_PLUGINS" ]; then
    echo "Grafana plugin list is current. No action required."
else
    echo
    echo "Grafana plugin list is not current."
    echo
    echo "If you have installed custom plugins, you should answer 'n' to the"
    echo "next question and you should update your installation manually."
    echo "If you are running a default install (no changes), you should answer"
    echo "'y' to the question and you should update your dashboard per the "
    echo "instructions at the end of this process."
    echo "Update will delete *ALL* current plugins and only download those"
    echo "on the current list."
    echo 
    read -r -p "Update grafana plugins? [y/N] " response
    if [[ "$response" =~ ^([yY][eE][sS]|[yY])$ ]]
    then
        sed -i.bak "s@GF_INSTALL_PLUGINS=.*@${GF_NEW_PLUGINS}@g" $GF_ENV_FILE
        rm -fr ./grafana/plugins/*
        docker stop grafana
        docker rm grafana
    else
        echo "No change."
    fi
fi

jasonacox commented 4 months ago

Thanks for this @BuongiornoTexas . I'm hesitant to make this a permanent part of our upgrade.sh script, always telling users "Grafana plugin list is not current" even if it is that they just added their own tweaks to grafana.env. If possible, we should have a list of plugins we suggest should be removed and if they are in the grafana.env file, ask if user wants them removed. Thoughts?

BuongiornoTexas commented 4 months ago

@jasonacox - I'm more than happy to take on suggestions and criticism - I put this rough solution together on the basis that I'm the trouble maker who raised the issue, so I should take some responsibility for resolving it! 😃

I did look at creating a list of non-core plugins and asking the user which ones they wanted to keep. This raises two problems: 1) it will be a relatively complicated piece of code for something that will be done infrequently (and upgrade.sh is already doing a lot!) and 2) we would need to add another config file to keep track of the plugins that each user wants to keep as part of their customisation.

Riffing on your suggestion, maybe we could do variation on the tricks Powerwall-Dashboard already does and get a less complicated implementation:

Add a new variable into grafana.env.sample e.g. CORE_PLUGINS=blah, blah2, ...
Change the grafana environment variable to: GF_INSTALL_PLUGINS=${CORE_PLUGINS}
Document that users should add custom plugins to the end of GF_INSTALL_PLUGINS
In upgrade.sh check CORE_PLUGINS if any old plugins have been removed.
- For each removal, ask the user if they want to retain the existing plugin. If yes, then add this plugin to the end of GF_INSTALL_PLUGINS.
- Otherwise silently delete the plugin from grafana.

From an end user perspective, this is pretty clean, as they only get the notice once per change. It also doesn't requiring separate tracking of user custom plugins (they stay in grafana.env). The only drawback is that it will still be a relatively complex bit of update code for something that will only be used infrequently. (and we also need to add CORE_PLUGINS to older grafana.env files ...).

I also note:

Most users will probably only use the reference set of plugins, and so will not be worried about an occasional upgrade request. And this group will see the upgrade notice rarely.
Any user adding new plugins is pretty well a power user by definition - I don't think they will be worried by a notice that they have additional plugins installed. (case in point - I'm not a hard core customiser, but I've probably done more than most people, and I'm still only using the core plugins.)

Finally, I was really focussing on mechanism rather than description - one of the issues you have touched on is message wording - this can easily be reworked into a more neutral phrasing.

BuongiornoTexas commented 4 months ago

Sorry - I should have said - I'm happy to draft either your suggestion or the alternate version using the environment variables for tracking.

jasonacox commented 4 months ago

Good points @BuongiornoTexas - I suggest we just change the language a bit. Perhaps:

You appear to be running an older Grafana configuration with plugins that are no longer
required. This script can update your Grafana plugins to the current recommended list. 
If you have installed custom plugins or want to keep it as it is, answer 'n' below.

Do you want to update your Grafana plugin list? (Y/n):

BuongiornoTexas commented 4 months ago

@jasonacox - I've ended up going down the rabbit hole of doing something along the lines of second solution tracking changes in the environment variables. If I can make it work, it should be a cleaner solution than my first pass. If I can't, I'll incorporate your wording into my original script.

It may take a bit of time to sort out - I'm running into similar problems as others with the rate limiting and the .44 firmware, Which I think I'll need to resolve before I can sort this part out.

On a side note, were you aware that the way Powerwall-Dashboard is using GF_INSTALL_PLUGINS will redownload and reinstall the plugins each time the grafana container is started up (I only found out by accident as my ISP is causing shenanigans with the google storage apis).

This was the post that clued me into the issue: https://community.grafana.com/t/grafana-docker-compose-downloads-plugins-on-every-start/10976

jasonacox commented 4 months ago

No rush, thanks @BuongiornoTexas .

were you aware that the way Powerwall-Dashboard is using GF_INSTALL_PLUGINS will redownload and reinstall the plugins each time the grafana container is started up

Yes, I thought that was the Grafana way. :) I'm open to suggestions on how to improve.

rate limiting and the .44 firmware

I'm not not on the new firmware so I'm at a disadvantage to help troubleshoot this. I know that the older pypowerwall version would non-stop query the /vitals even if it was a 404 causing rate limiting, but that was fixied in https://github.com/jasonacox/pypowerwall/releases/tag/v0.7.6 and https://github.com/jasonacox/Powerwall-Dashboard/releases/tag/v3.0.8. If you are able to discover the cause, let let me know so we can get a fix in.

BuongiornoTexas commented 4 months ago

No worries - slow progress on the plugin then, and I'll follow up on making plugins persistent as well (I don't like the grafana way - makes my containers slow/unreliable to start).

On the 0.44, I'll spin up a local pypowerwall and see what I can find. Which discussion/issue would you prefer to have reporting on 0.44 issues?

jasonacox commented 4 months ago

No worries on the plugin side. I suspect the Firmware issues are a higher priority.

We can use the latest issue for updates https://github.com/jasonacox/Powerwall-Dashboard/issues/436 and I'll combine with https://github.com/jasonacox/Powerwall-Dashboard/issues/425.

BuongiornoTexas commented 4 months ago

OK - back to the plugins. I've pulled together an implementation that does plugin version control lite using environment variables. I'll submit it as a PR - it's quite a lot of code for pretty small functionality, so: a) if you'd prefer to avoid the complexity, I'm quite happy to revert to the first option. b) I've implemented the code as a separate file which I suggest can be kept in the tools file and can just be sourced from update.sh. Should be in place for review today/tomorrow.

jasonacox commented 4 months ago

Thanks @BuongiornoTexas - My bias is going to be toward "as simple as possible". And in a simliar vein, I would also prefer to not create more external file/script dependencies if we can avoid it. However, making a stand alone tool (not called by upgrade) is not a bad idea.

BuongiornoTexas commented 4 months ago

making a stand alone tool (not called by upgrade) is not a bad idea.

I don't feel strongly about this, but I don't think providing this as a manual tool adds much value. We still need to identify when it needs to be run and then tell the user how to run it. And given it is doing a few small edits to a config file, we might as well just provide instructions on the edits needed rather than instructions on running the script. (it's fine for power users, but they are the group who don't need it at all)

My bias is going to be toward "as simple as possible".

The more complex script adds nearly 50% to the length of update.sh so I don't think you'll want to add this in directly.

I've realised that my original script does a poor job on handling additions to the plugin list - the case where we really do need to update the plugin list! I'm having a think about how to address this without getting code bloat back to the second version.

BuongiornoTexas commented 4 months ago

OK, hopefully the last pass. I think this is an 80/20 solution, where the changes are as simple as I can make them and the pain should mainly fall on power users who add new plugins. This fix does not catch edge cases, and particularly does not catch duplicates between core and custom plugins - I don't think this is an issue, as all grafana will do is download the plugin twice.

It is robust to future changes to core plugins, and I think the solution can entirely replace the ad hoc check for the yesoreyeram boomtable section that sits in upgrade.sh at the moment.

If you are happy with the proposal, I'll convert to a PR.

1) Update grafana.env.sample, replacing the GF_INSTALL_PLUGINS=.* line with the following lines.

# Core plugins required by Powerwall-Dashboard managed in PWD_CORE_PLUGINS. 
# Do not edit this - managed by setup.sh & upgrade.sh.
# WARNING - do not redefine this variable later in this file - it will break upgrade.sh
PWD_CORE_PLUGINS=grafana-piechart-panel, https://github.com/yesoreyeram/yesoreyeram-boomtable-panel/releases/download/v1.5.0-alpha.3/yesoreyeram-boomtable-panel-1.5.0-alpha.3.zip;yesoreyeram-boomtable-panel, fetzerch-sunandmoon-datasource, simpod-json-datasource

# Add your customs plugins to the end of GF_INSTALL_PLUGINS (comma-space separated!).
GF_INSTALL_PLUGINS=${PWD_CORE_PLUGINS}

2) Add most of this script into update.sh. The run once check can probably be removed 6-12 months down the track when most active users have updated to current versions (I think this will be forced by the Tesla firmware updates).

#!/bin/bash

# already defined in upgrade.sh.
GF_ENV_FILE="grafana.env"

# With the new block in place, I suspect we can safely delete the
# yesoreyeram-boomtable block that current sits in upgrade.sh
# on lines 199-212.
# (2.6.2 was a while ago, and I think this change supercedes it).

# Insert this block after line 213 in upgrade.sh

# Create variables as script gets unwieldy otherwise
core_env=PWD_CORE_PLUGINS
old_core=$(sed -n "s/^${core_env}=//p" ${GF_ENV_FILE})
new_core=$(sed -n "s/^${core_env}=//p" ${GF_ENV_FILE}.sample)

if [ "${old_core}" == "" ]; then
    # This if block is a run once - could move to a separate tool,
    # or just delete 6 or 12 months in the future, as everyone '
    # will probably have been forced to update by then due to
    # Tesla firmware updates. 

    echo "Updating '${GF_ENV_FILE}' structure. '.old' backup file created."

    old_core=$(sed -n "s/^GF_INSTALL_PLUGINS=//p" ${GF_ENV_FILE})
    update_gf_file="# Plugins updated to use ${core_env}.\n# See '${GF_ENV_FILE}.sample' for details."
    update_gf_file="${update_gf_file}\n${core_env}=${old_core}"
    update_gf_file="${update_gf_file}\nGF_INSTALL_PLUGINS=\${${core_env}}" 

    # Force update of grafana env file.
    sed -i.old -e "s@^GF_INSTALL_PLUGINS=.*@${update_gf_file}@" $GF_ENV_FILE
fi

if [ "${old_core}" == "${new_core}" ]; then
    echo "Grafana plugin list is current. No action required."
else
    echo
    echo "Grafana plugin list has changed."
    echo
    echo "You appear to be running an older Grafana configuration with"
    echo "plugins that are different to the current recommended list."
    echo "This script can update your plugins to the current recommendations."
    echo "If you have installed custom plugins or want to keep it as"
    echo "it is, answer 'n' below and the script will provide instructions for"
    echo "manual checks/updates."
    echo 
    read -r -p "Do you want to update your Grafana plugin list? [y/N] " response
    if [[ "$response" =~ ^([yY][eE][sS]|[yY])$ ]]
    then
        sed -i.bak -e "s@^${core_env}=.*@${core_env}=${new_core}@" $GF_ENV_FILE
        docker stop grafana
        docker rm grafana
        # Probably safe to delete all plugins as grafana downloads everything everytime anyway.
        rm -fr ./grafana/plugins/*
    else
        echo
        echo "    Okay. Your grafana plugin list has not been updated."
        echo ""
        echo "However, this version of Powerwall-Dashboard may need new plugins"
        echo "for the latest grafana dashboard. To address this, please"
        echo "check the list of required plugins (listed in ${core_env} in"
        echo "'${GF_ENV_FILE}.sample') and make sure all of these plugins are"
        echo "added to ${core_env}' in '${GF_ENV_FILE}'."
    fi
fi

jasonacox commented 4 months ago

Thanks @BuongiornoTexas ! Elegant and simple. Please copy that into a PR so we can put it through some tests. 🙏

jasonacox / Powerwall-Dashboard

Minor update: remove flux datasource plugin from grafana #433