jasonacox / Powerwall-Dashboard

Grafana Monitoring Dashboard for Tesla Solar and Powerwall Systems
MIT License
303 stars 66 forks source link

Use raw variable for timezone #439

Closed s-crypt closed 8 months ago

s-crypt commented 9 months ago

When I updated my local version of grafana (for "fun"), most of the queries broke, and thanks to the info in #431, I figured out that at some point variable referencing changed. Grafana inserts an escape \ into the timezone variable, like America\/Los_Angeles which breaks most of the panels' queries. This results in the error InfluxDB returned error: error parsing query: found \/, expected identifier...

Using ${tz:raw} instead of $tz ensures the variable is output as-is. This will make upgrading Grafana easier later on and future-proof the variables.

I tested and had no issues using ${tz:raw} on the main dashboard in grafana 9.1.2 and 10.3.3.

mcbirse commented 8 months ago

Great find @s-crypt !

I have tested and confirm I was able to replicate this issue with Grafana 10.3.3, and that using ${tz:raw} instead appears to solve the issue.

Interestingly, I thought this may just be an issue introduced to the dashboard when you perform an upgrade to 10.3.3... and that re-importing dashboard.json would then resolve it. However, in my testing the problem still occurs even when you re-import the dashboard, so it seems the solution is to change all references to ${tz:raw} (also backward compatible to Grafana 9.1.2). This almost seems like a bug with Grafana to me...

I checked the docs regarding the advanced syntax for variables and the raw clause which says using "raw" returns the UID (unique identifier) of the data source, rather than its name. I find this a bit confusing as there is no UID for the TZ variable? Strange - it works regardless, so I assume must return the unescaped text value of the variable instead.

In saying that, I found that using ${tz:text} also works and resolves the issue. I wonder if it is better to use this since the TZ is expected to be a text variable?

I am still testing this before we decide to merge, as I think we could revise the use of tz() in queries in many cases as well. Looking into this, it appears to me tz() in the majority of the queries is actually unnecessary.

Would welcome some feedback on this... but from what I can tell adding a the tz() function to the query is only really necessary when also doing a GROUP BY (1d) (or similar), which only occurs in a few panels (e.g. Monthly Analysis or Solar Energy Year). Otherwise, it does not appear to be required and probably could be removed.

jasonacox commented 8 months ago

Thanks @mcbirse ! Let us know how your testing goes.

Would welcome some feedback on this... but from what I can tell adding a the tz() function to the query is only really necessary when also doing a GROUP BY (1d) (or similar), which only occurs in a few panels (e.g. Monthly Analysis or Solar Energy Year). Otherwise, it does not appear to be required and probably could be removed.

I suspect you are right. There are some simple copy/paste laziness or even vestigial left overs in some of these queries that date back to the beginning. It would be good to clean up if we can.

s-crypt commented 8 months ago

I switched tz:raw to tz:text, and I also removed $tz from any queries that do not have a GROUP BY statement using this regex ((?="query":.+?)(?:(?!GROUP BY).)*)(tz\('\${tz:text}\'\)).

mcbirse commented 8 months ago

Thanks @s-crypt - I will review/test the updates this weekend hopefully. Have not had much spare time lately.

jasonacox commented 8 months ago

Thanks @s-crypt and thanks @mcbirse ! 🙏

I merged with the 4.0.4 updates and labeled this 4.0.5. I also included the pypowerwall update that will fixes the power flow animation for solar-only owners in case they want to use that on their dashboards.

@s-crypt - great addition! Thanks for PR.