Skip to content

first pass - #127

Open
Hellowlol wants to merge 21 commits into
masterfrom
fix-1
Open

first pass#127
Hellowlol wants to merge 21 commits into
masterfrom
fix-1

Conversation

@Hellowlol

@Hellowlol Hellowlol commented Mar 21, 2022

Copy link
Copy Markdown
Collaborator

This PR is an attempt to make the integration more resilient against Nordpool API errors, by retrying on HTTP errors and missing/wrong data with exponential retries.

Comment thread custom_components/nordpool/__init__.py Outdated
@yozik04

yozik04 commented Mar 23, 2022

Copy link
Copy Markdown

With the version from yesterday there is a bug that it keeps the same prices for tommorow.
Today
0.171, 0.179, 0.175, 0.173, 0.175, 0.116, 0.241, 0.32, 0.348, 0.329, 0.252, 0.234, 0.223, 0.209, 0.148, 0.132, 0.093, 0.132, 0.27, 0.306, 0.306, 0.277, 0.267, 0.277
Tomorrow
0.171, 0.179, 0.175, 0.173, 0.175, 0.116, 0.241, 0.32, 0.348, 0.329, 0.252, 0.234, 0.223, 0.209, 0.148, 0.132, 0.093, 0.132, 0.27, 0.306, 0.306, 0.277, 0.267, 0.277

Raw today and Raw tomorrow have both same data, same timestamps, same values.

Comment thread custom_components/nordpool/__init__.py Outdated
@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

2022-04-02 10:37:35 WARNING (MainThread) [homeassistant.components.sensor] Setup of sensor platform nordpool is taking over 10 seconds.
....
2022-04-02 10:41:26 WARNING (MainThread) [homeassistant.bootstrap] Waiting on integrations to complete setup: sensor.nordpool

Starts already for 4 minutes. something is wrong there...

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

Please enable the debug log and the log. The sensor can take a long time depending on the issues it has

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

The integration made 69 requests to finally get data. i think it fails if prices are requested before midday (when new prices are published)
See log attached.
nordpool-fix-1_full.log

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

@yozik04 Thanks for the debug log! It was really helpful, the problem was that the retry method considers all "falsy" as a request for retrying. Ill fix that :)

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

The method you used to make nordpool library async is very scary. I'd better throw a PR to add async to that library. =)

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

BTW. Take a look to the latest version of that library. It already has async support.

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

Ahh, you have just made a copy of that async version and modified... Ok, your code, your rules...

@Hellowlol

Hellowlol commented Apr 2, 2022

Copy link
Copy Markdown
Collaborator Author

@yozik04 The API wasn't async when I made this, I was the one that sent an upstream PR to make it async. I kept my code to make sure that it worked. See PR kipe/nordpool#8 and kipe/nordpool#11 If you have a better way to make this async while supporting multiple async HTTP libraries, feel free to send a PR

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

@yozik04 I think it's ready to be merged. Can you test and leave it running until the prices for Monday should be available?

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

@Hellowlol I will. Updating

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

It still does 6 requests to nordpool on HA restart.

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

Updating todays prices and Updating tomorrows prices. 3 requests each.

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

@yozik04 thats intended. The api stores/get the data in cet. So if you requested data area in another time zone you wouldn’t get the correct prices. That’s why we request data for 3 days and extract what we need.

@yozik04

yozik04 commented Apr 2, 2022

Copy link
Copy Markdown

It is ok to fetch 3. But why it fetches 3 requests two times.

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

Its one request for today and one for tomorrow.

@yozik04

yozik04 commented Apr 3, 2022

Copy link
Copy Markdown

Found infinty invalid data in area SYS
Why it checks SYS if I am interested in EE only?

Looks like in my region prices appear at 15:00 UTC+3. For some reason it started requesting at 14:00, which is 11 UTC.

But overall it works. Actually not. See below.

@yozik04

yozik04 commented Apr 3, 2022

Copy link
Copy Markdown

Ahh, my father has restarted HA in the morning. He told it showed today and tomorrow same prices...

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

@yozik04 can you post the log? I want to check why it started requesting data then

@Hellowlol

Hellowlol commented Apr 3, 2022

Copy link
Copy Markdown
Collaborator Author

I will drop an idea here how I would do the whole thing.
`
I'd create a class that would hold all fetched valid values (yesterday, today, tomorrow) in a single dictionary (datetime as a key). Let's name it PriceData That class would have methods:

Thanks for your suggestion, that is pretty much how this is designed today with some small changes to be able to handle currencies and we also need to clear the sensors data in addition to NordpoolData as we can't do IO in a property.

  • get_todays_data - would scan the dictionary and output todays's prices today
  • get_tomorrows_data - would scan the dictionary and output tomorrow's prices tomorrow
  • has_todays_data - would tell if dictionary has todays data | Does not exist
  • has_tomorrows_data - would tell if dictionary has tomorrows data | Does not exist
  • clear_expired - would scan the dictionary and drop data that is already expired new_day_cb
  • store_data - injects the data to the dictionary (actually a nice place to validate the data as well, and inject only valid values) _update
  • get_current_price - probably it is also needed, you know better. | Cant be handle here, must be handled in sensor

Code out of PriceData class would query nordpool for today's data only if PriceData.has_todays_data returns False. Same with tomorrow but should also verify that time is adequate for the request. If fetch was successful and data is valid we can still reuse has_todays_data, has_tomorrows_data

Every day something needs to call PriceData.clear_expired as well (probably run it before every request to nordpool). new_day_cb

That would be my design. I would even cover PriceData with some unit tests. But I do not really want to go and change your code. =) No offence.

I'll accept every PR that makes the sensor more reliable as long as none of the current features are missed. 😊

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

Ill add some tests

@yozik04

yozik04 commented Apr 4, 2022

Copy link
Copy Markdown

At night both day prices became unavailable with new version. Will send logs later.

@yozik04

yozik04 commented Apr 4, 2022

Copy link
Copy Markdown

Todays log: nordpool-2022-04-04.log

@yozik04

yozik04 commented Apr 4, 2022

Copy link
Copy Markdown

yesterday's logs were lost unfortunately.

@Hellowlol

Hellowlol commented Apr 4, 2022

Copy link
Copy Markdown
Collaborator Author

Thanks, that was helpful. 👍 The server was restarted after midnight so there isn't any available data for tomorrow, it will try one then, stop. However, it also did a new HTTP call to get today's data. There were invalid values in FI results, so the requests for retried over and over until the setup of the senor failed. We validate on currency as the NordpoolData don't know what data the user wants

@yozik04

yozik04 commented Apr 5, 2022

Copy link
Copy Markdown

nordpool_2022-04-04_05.log
Worked fine today on version before tests. Posting log for reference.

@yozik04

yozik04 commented Apr 6, 2022

Copy link
Copy Markdown

I can confirm the last one is working as expected.

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

@yozik04 Can you do one last test? I was thinking of making a new release this weekend.

@yozik04

yozik04 commented Apr 6, 2022

Copy link
Copy Markdown

Pulled latest changes.
I have 00:37 on the clock now and it failed to download/update the data. Found infinity inside FI... Will see if it will recover during the night.
As before, at the night time it fails. Due to Nordpool returning junk or an other reason. Anyway I will send logs tomorrow. I have not digged what is in the returned XML yet. There are some valid values for sure.

if the user has multiple regions all of them have to be valid
@Hellowlol

Copy link
Copy Markdown
Collaborator Author

The latest update should only verify the area that the user requested. If the user has multiple sensors using the same currency, then all of the data for the selected areas have to be valid.

@yozik04

yozik04 commented Apr 7, 2022

Copy link
Copy Markdown

Logs from version 34be90f
nordpool-2022-04-07_night.log

JSON files that were fetched at night:
08-04-2022.txt
07-04-2022.txt
06-04-2022.txt

# 2022-04-07 00:33:08 DEBUG (MainThread) [custom_components.nordpool.aio_price] requested https://www.nordpoolgroup.com/api/marketdata/page/10?currency=EUR&endDate=06-04-2022 {'currency': 'EUR', 'endDate': '06-04-2022'}
cat 06-04-2022.txt | jq -c '[.data.Rows[].Columns | map(select(.Name == "EE"))[].Value]'
["81,73","72,17","69,23","71,48","74,79","74,13","87,07","136,61","151,41","95,96","90,42","87,16","86,68","84,67","84,90","86,91","87,68","87,63","87,91","104,52","111,75","54,68","45,08","27,35","27,35","151,41","85,08","94,65","83,40","59,72"]

# 2022-04-07 00:33:08 DEBUG (MainThread) [custom_components.nordpool.aio_price] requested https://www.nordpoolgroup.com/api/marketdata/page/10?currency=EUR&endDate=07-04-2022 {'currency': 'EUR', 'endDate': '07-04-2022'}
cat 07-04-2022.txt | jq -c '[.data.Rows[].Columns | map(select(.Name == "EE"))[].Value]'
["81,73","72,17","69,23","71,48","74,79","74,13","87,07","136,61","151,41","95,96","90,42","87,16","86,68","84,67","84,90","86,91","87,68","87,63","87,91","104,52","111,75","54,68","45,08","27,35","27,35","151,41","85,08","94,65","83,40","59,72"]

# 2022-04-07 00:33:08 DEBUG (MainThread) [custom_components.nordpool.aio_price] requested https://www.nordpoolgroup.com/api/marketdata/page/10?currency=EUR&endDate=08-04-2022 {'currency': 'EUR', 'endDate': '08-04-2022'}
cat 08-04-2022.txt | jq -c '[.data.Rows[].Columns | map(select(.Name == "EE"))[].Value]'
["-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-","-"]

As you can see 6th April's data is absolutely equal to 7th April, what is bullshit. 8th is not available.

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

Thanks again! Clearly it’s bug in the retry. However I have no idea why the values for today and tomorrow was the same. How if you get the json? Did you do manual request and download it? The raw json isn’t logged in this integration.

@yozik04

yozik04 commented Apr 7, 2022

Copy link
Copy Markdown

I have copied urls from the HA log and used wget at that time.

@poffe

poffe commented May 2, 2022

Copy link
Copy Markdown

Hi! Just a friendly poke to see how this is progressing? I've seen this issue a couple of times so that's the reason. Great work btw.

@Hellowlol

Copy link
Copy Markdown
Collaborator Author

I haven't forgotten this. I just got frustrated with the API returning junk :P It will get merged sometime in the future :D

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.

4 participants