Handle LastFM unavailable #94456
Handle LastFM unavailable #94456
Conversation
home-assistant
bot
added
bugfix
cla-signed
integration: lastfm
small-pr
|
I am a bit in doubt on how to solve the unavailable backend, as we do 5 calls to update the state for one sensor. If one of those calls now fail we set it to unavailable. Is this a good way to handle unavailable data? Are there better ways? |
| ATTR_PLAY_COUNT: play_count, | ||
| ATTR_TOP_PLAYED: top_played, | ||
| } | ||
| except PyLastError as exc: | ||
| self._attr_available = False | ||
| LOGGER.error("Failed to load LastFM user `%s`: %r", self._user.name, exc) |
There was a problem hiding this comment.
Perhaps we could rethrow this with some kind of HA core error? I think it would better indicate to HA that there's a problem and I think it'll do retries for us?
There was a problem hiding this comment.
Previously we used get_playcount to verify a user still exists. So if a user got deleted it would become unavailable. But now I want to handle what happens if LastFM is having connection issues. So throwing a Home Assistant error wouldn't be fitting as it's an error originating from LastFM
|
I know it's still a draft, but don't forget tests :) |
Perhaps it would be good to only set |
Better leaving them at previous value on second thought. |
|
Hmm, for some reason the tests doesn't cover the except. (Altho the sensor.py doesn't have SCAN_INTERVAL, so 15 minutes would be too high, but tried locally and that didn't matter). Any ideas what I can try? |
| try: | ||
| self._attr_entity_picture = self._user.get_image() | ||
| if now_playing := self._user.get_now_playing(): | ||
| self._attr_native_value = format_track(now_playing) | ||
| else: | ||
| self._attr_native_value = STATE_NOT_SCROBBLING | ||
| top_played = None | ||
| if top_tracks := self._user.get_top_tracks(limit=1): | ||
| top_played = format_track(top_tracks[0].item) | ||
| last_played = None | ||
| if last_tracks := self._user.get_recent_tracks(limit=1): | ||
| last_played = format_track(last_tracks[0].track) | ||
| self._attr_extra_state_attributes = { | ||
| ATTR_LAST_PLAYED: last_played, | ||
| ATTR_PLAY_COUNT: play_count, | ||
| ATTR_TOP_PLAYED: top_played, | ||
| } | ||
| except PyLastError: | ||
| return |
There was a problem hiding this comment.
This feels like a large try scope? As in, if get_top_tracks fails, a user is left with a partial update? While get_recent_tracks isn't even tried anymore?
The scope of the try should be limited to a single call IMHO; or handle the unavailable state (in which all values become unavailable).
There was a problem hiding this comment.
When I look at my logs this happened like 20 times in the last 4 days. What should the preferred option be? In my opinion, the best way would be to split the data up into several sensors so we can put it unavailable independently, but that'd be a breaking change. So would putting everything in one try except be the best solution for now?
There was a problem hiding this comment.
I think I would implement the available state right now, and make the sensor unavailable. As a follow-up PRs:
- Implement a data coordinator
- Add sensor descriptions
- Add separate sensors for the attributes
- Keep the attributes for the entity around for a couple of release cycles before removing them from the old sensor entity.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
frenck
added
the
smash
|
@joostlek tests are failing. Can you take a look? |
Proposed change
Apparently LastFM can return an error while updating a sensor (so at the first verify where we check if the user exists (get_playcount) until the last playcount call). It will throw an exception in the log that it couldn't update the sensor.
In this change I am just throwing everything in the try except to handle this behaviour.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: