Performance and functional improvements

[isorin]

Code changes to:

1. Improve the performance of the code
   The existing code is highly inefficient as it searches through entire lists of past forecasts (up to 2 years) every minute to update all sensors
   I made the following performance improvements:

• Removed the past forecasts from memory (the history is still saved as before) when calculating the sensors and optimised the searches to only look for the necessary data.
• Reduced the update interval of sensors to 5 minutes
• Split the sensors into two groups: sensors that need to be updated every 5 minutes and sensors that need to be updated only when the data is refreshed or the date changes (daily values)
• Fixed issues with removing the past forecasts (older than 2 years), the existing code is just wrong (uses an iterator to remove from the list while traversing it)
• Needless to say that the efficiency of the code is improved dramatically.

2. Improve the functionality of the forecasts
   The existing code, doesn't use any "intelligence" in calculating the sensor values, it merely sums the 30 minutes intervals that align with the current date/time, it updates all sensors every minute with the same values for 30 minutes and then goes to the next interval and so on.
   I refactored the sensor calculation logic so it would update the "time sensitive" sensors every 5 minutes.
   For instance, "forecast_remaining_today" is properly updated every 5 minutes by calculating the remaining energy from the current 30 minute interval. Same for "now/next hour" sensors.

I use the "forecast_remaining_today" to determine the time of the day when to start charging the batteries so that they will reach a predetermined charge in the evening. With my changes, this is possible.

[BJReplay]

Thanks for the PR, Sorin

I will also ask @autoSteve to review.

A couple of notes:

It looks like there might be some changes that @autoSteve has been making that are conflicting, in particular around 231 in your PR vs 214 in v3.

I can see from the PR commit history that you pulled in changes from v3 late, and as you've probably seen, Steve has been chasing a few bugs related to implementing oziee's original v4.0.23 changes that had some issues.

Again, thank you for the significant PR, and I'll wait for @autoSteve's review.

[isorin]

No problem, I'll look into syncing with his changes tomorrow.On 19 Jun 2024 00:24, BJReplay ***@***.***> wrote: @BJReplay commented on this pull request. Thanks for the PR, Sorin I will also ask @autoSteve to review. A couple of notes: It looks like there might be some changes that @autoSteve has been making that are conflicting, in particular around 231 in your PR vs 214 in v3. I can see from the PR commit history that you pulled in changes from v3 late, and as you've probably seen, Steve has been chasing a few bugs related to implementing oziee's original v4.0.23 changes that had some issues. Again, thank you for the significant PR, and I'll wait for @autoSteve's review. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[isorin]

I see changes in 3 different branches, I will wait for @autoSteve to review and decide what's the best strategy to merge the changes. I can rebase my PR once all other changes are merged.

[autoSteve]

Everything required is now merged to the v3 branch, @isorin. There was a little branch mayhem for a while, but it's flattened out.

[isorin]

Everything required is now merged to the v3 branch, @isorin. There was a little branch mayhem for a while, but it's flattened out.

@autoSteve Nice, I'll start working on syncing the changes and updating the PR

[isorin]

@BJReplay @autoSteve
PR is ready for review and merge, I think I'm done with the changes.

(Hope my few changes related to "api usage cache file" are fine, I tested by simulating a multi-site as I only have one site)
One thing to mention, there was no update of the cache file for the reset at midnight, I added one.

Feel free to ask me anything about my code changes.
At this stage I know the code quite well and I've been using it (with my changes) for a few weeks.

[BJReplay]

I am going to respectfully request that @autoSteve take a look at the PR after he has had a few days to decompress from the last release.

My thoughts are that, subject to review, that once the PR is merged, that we release as a re-release again, and only when stable, promote to release.

[isorin]

Ah, no worries and no rush with the review, as I said I'm using it from my own fork, I created a new release with all the changes combined:
https://github.com/isorin/ha-solcast-solar/releases/tag/v4.0.31.1

[autoSteve]

Elegantly done, @isorin.

Nice to see you even picked up a logger call that was never going to happen. And also that you picked up on the no cache write at midnight UTC reset. I like these changes so much I'm running your current 4.0.31.1 release.

Fixed issues with removing the past forecasts

I wondered how inefficient that approach was, and even whether it worked before, so thanks for doing it properly.

Lets's wait for any fallout from the pre-release v4.0.32 @BJReplay before merge. No complaints so far about that beta.

[autoSteve]

@isorin, I have on the to-do list redaction of the API key in debug logs.

This could be accomplished by using something like '*'*26+apikey[26:] wherever the key is logged. By doing so, it is clear which key is being referred to for the actions logged, while not showing the whole key.

Oziee chose to put a simple REDACTED in the log, but I exposed the key with cache additions, then you carried that exposure further by logging the cache file name, which would expose it for multi-account use. It would be brilliant if this pull solved that.

Redaction will ensure that any logs submitted do not expose any API keys, eliminating the possibility of nefarious activity like a bad actor repeatedly using up a key's API calls every day because they're p*ssed at someone.

[isorin]

@autoSteve
I pushed the changes to redact the API keys in logs.
I made the strings shorter, 6 * chars instead of 26 (e.g. ******abc123) to denote a key ending in 'abc123' with some chars obfuscated. I don't think it matters that we match exactly the number of chars in the key but if you feel strongly about this, let me know and I can update the code.
I hope I got all places where the api key is logged.

Re. logger issue:
The "logger call that was never going to happen" happened to me once :) this is how I knew it was broken.

Re. performance:
When I started looking at the code back in April I already had accumulated around 3 months of history and after making the changes I calculated an increase in performance of about '50x'.
Not only that the performance was bad, but the sensors were updated many times (1 time per minute) with the same values as the code only looked at the 30minutes intervals as a whole (i.e. either use it or discard it).
Many sensors like "forecast per day" were updated every 1 minute with the same values for the entire day.

[isorin]

@BJReplay @autoSteve
Anybody has multi-site to test the changes properly? I only have single site.

[autoSteve]

@ffm777 has four rooftops, two accts, and has been testing v4.0.32 pre-release.

[autoSteve]

The API key redactions look good to me @isorin. Thanks for taking the time to squeeze that in there.

[autoSteve]

Here's another redaction challenge, @isorin, if you're up for it. I just gave out my home address in an issue report. 🤦🏻

The "sites data" raw JSON is logged, and this includes latitude and longitude of the site. That issue report will be killed, re-created, and redacted.

🤦🏻🤦🏻🤦🏻

[autoSteve]

@BJReplay, I'd appreciate it if you could please delete that issue when you get a chance. The closed issue "https://github.com/BJReplay/ha-solcast-solar/issues/56".

[isorin]

Here's another redaction challenge, @isorin, if you're up for it. I just gave out my home address in an issue report. 🤦🏻

The "sites data" raw JSON is logged, and this includes latitude and longitude of the site. That issue report will be killed, re-created, and redacted.

🤦🏻🤦🏻🤦🏻

I'll see what time I can find to look at it this week.
We should probably make the changes as part of a new PR and close this one as is.

[BJReplay]

@BJReplay, I'd appreciate it if you could please delete that issue when you get a chance. The closed issue "https://github.com/BJReplay/ha-solcast-solar/issues/56".

Hmmm, will try from desktop later but no luck from mobile github client to delete issue. Is there a comment to redact instead?

[autoSteve]

Is there a comment to redact instead?

Thanks @BJReplay. It's edited and redacted, but I don't want anyone to get any ideas. I'm also unsure whether edit history can be uncovered from Github, so in the fullness of, and in no hurry, just kill it please. It probably went to watcher inboxes, but what evs. Just don't want stuff like this online forever.

[BJReplay]

Deleted

[autoSteve]

This should do us for lat/long redaction. The resource ID is only accessible with the API key, so not really leakage. This is not in a code block for readability. (edit: And now it is unreadable. Ugh. In a code block.) (edit) Changed to asterisks for consistency.

I'll include in a post 4.0.33 PR. Uses a re.sub(r'itude\': [0-9\-\.]+', 'itude\': **.******', responseText).

sites_data: {'sites': [{'name': 'Iris North-East', 'resource_id': 'b68d-c05a-c2b3-2cf9', 'capacity': 6.857,
'capacity_dc': 7.92, 'longitude': **.******, 'latitude': **.******, 'azimuth': -67, 'tilt': 26, 'loss_factor': 0.95, 'tags': []}, {'name': 'Iris North-West', 'resource_id': '83d5-ab72-2a9a-2397', 'capacity'
: 5.243, 'capacity_dc': 5.72, 'longitude': **.******, 'latitude': **.******, 'azimuth': 27, 'tilt': 17, 'loss_factor': 0.95, 'tags': []}], 'page_count': 1, 'current_page': 1, 'total_records': 2}

[autoSteve]

I see no reason why these code changes can't be merged with v3, then update README.md / manifest.json and publish as pre-release v4.0.33. This code has been rock solid on my 'prod' instance for days, and is well better than its predecessor.

Just say the word, @BJReplay, and it shall be done.

[BJReplay]

Make it so.

[autoSteve]

#trekkie.