Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

first pass #127

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

first pass #127

wants to merge 21 commits into from

Conversation

Hellowlol
Copy link
Collaborator

@Hellowlol Hellowlol commented Mar 21, 2022

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.

@yozik04
Copy link

yozik04 commented Mar 23, 2022

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.

@yozik04
Copy link

yozik04 commented Apr 2, 2022

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
Collaborator Author

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

@yozik04
Copy link

yozik04 commented Apr 2, 2022

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
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
Copy link

yozik04 commented Apr 2, 2022

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
Copy link

yozik04 commented Apr 2, 2022

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

@yozik04
Copy link

yozik04 commented Apr 2, 2022

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

@Hellowlol
Copy link
Collaborator Author

Hellowlol commented Apr 2, 2022

@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
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
Copy link

yozik04 commented Apr 2, 2022

@Hellowlol I will. Updating

@yozik04
Copy link

yozik04 commented Apr 2, 2022

It still does 6 requests to nordpool on HA restart.

@yozik04
Copy link

yozik04 commented Apr 2, 2022

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

@Hellowlol
Copy link
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
Copy link

yozik04 commented Apr 2, 2022

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

@Hellowlol
Copy link
Collaborator Author

Its one request for today and one for tomorrow.

@yozik04
Copy link

yozik04 commented Apr 3, 2022

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
Copy link

yozik04 commented Apr 3, 2022

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

@Hellowlol
Copy link
Collaborator Author

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

@Hellowlol
Copy link
Collaborator Author

Hellowlol commented Apr 3, 2022

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
Collaborator Author

Ill add some tests

@yozik04
Copy link

yozik04 commented Apr 4, 2022

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

@yozik04
Copy link

yozik04 commented Apr 4, 2022

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

@yozik04
Copy link

yozik04 commented Apr 4, 2022

yesterday's logs were lost unfortunately.

@Hellowlol
Copy link
Collaborator Author

Hellowlol commented Apr 4, 2022

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
Copy link

yozik04 commented Apr 5, 2022

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

@yozik04
Copy link

yozik04 commented Apr 6, 2022

I can confirm the last one is working as expected.

@Hellowlol
Copy link
Collaborator Author

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

@yozik04
Copy link

yozik04 commented Apr 6, 2022

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
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
Copy link

yozik04 commented Apr 7, 2022

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
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
Copy link

yozik04 commented Apr 7, 2022

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

@poffe
Copy link

poffe commented May 2, 2022

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
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