Opower does not handle multiple meters on a single account.

c0mputerguru commented 10 months ago

The problem

I'm using the opower integration with PSE to try to pull in utility usage. I have a non-standard setup as I have solar panels installed, so my account has 2 meters; 1 for grid consumption, and 1 for production. However the way the integration is written, it assumes that the tuple of subdomain, meter type and utility account id is unique, and in my case it is not (https://github.com/home-assistant/core/blob/a385ca93bd0d5b7de4b8b20fe7cc34c006ab1aed/homeassistant/components/opower/coordinator.py#L93-L100).

As a result, I see the following in my logs:

2024-01-13 02:53:36.696 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating sensor data with: []
2024-01-13 02:53:36.696 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_elec_4000542376_energy_cost and opower:pse_elec_4000542376_energy_consumption
2024-01-13 02:53:38.269 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_elec_4000542376_energy_cost and opower:pse_elec_4000542376_energy_consumption
2024-01-13 02:53:38.967 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_gas_4000542390_energy_cost and opower:pse_gas_4000542390_energy_consumption
2024-01-13 02:53:40.209 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Finished fetching Opower data in 6.903 seconds (success: True)

Note that there are 2 lines for updating opower:pse_elec_4000542376. Going to the underlying get customer API that is providing the data, there are 3 "accounts" present:

...
            "utilityAccounts": [
                {
                    "id": 3232041,
                    "uuid": "1f231471-d223-11e3-ba8c-1b40f3043709",
                    "utilityAccountId": "4000542376",
                    "utilityAccountId2": null,
                    "servicePointId": 373123,
                    "meterType": "ELEC",
                    "preferredUtilityAccountId": "4000542376",
                    "readResolution": "QUARTER_HOUR"
                },
                {
                    "id": 6729658,
                    "uuid": "699c98e6-b860-11e8-8151-1661d329c91b",
                    "utilityAccountId": "4000542376",
                    "utilityAccountId2": null,
                    "servicePointId": 2134661,
                    "meterType": "ELEC",
                    "preferredUtilityAccountId": "4000542376",
                    "readResolution": "QUARTER_HOUR"
                },
                {
                    "id": 3212155,
                    "uuid": "1f04a9f9-d223-11e3-ba8c-1b40f3043709",
                    "utilityAccountId": "4000542390",
                    "utilityAccountId2": null,
                    "servicePointId": 201726,
                    "meterType": "GAS",
                    "preferredUtilityAccountId": "4000542390",
                    "readResolution": "QUARTER_HOUR"
                }
            ]
...

Note that the 2 electric meter types have the same utility account id, but different service point ids. That seems to be the human readable distinguishing element between the 2 data streams.

I think there are 2 potential fixes:

Add the service point id as a part of the opower library (https://github.com/tronikos/opower/pull/65) and then add the service point id as an element of the id prefix. Hope that the addition of the service point id makes the id unique enough.
Use the uuid that is already present instead of the account id. Given that the field is called uuid, it seems reasonable to assume it will be unique.

The 1st option is better in terms of human readability, but might have some weird edge cases; I'm not sure how all providers populate the service point id and there isn't any API documentation to rely on. The 2nd is safer, but less human friendly. Both would be backwards incompatible changes. There is a 3rd option which is to add the uuid or service point id only if the existing prefix is not unique enough that would be backwards compatible, but the code for that could end up being pretty ugly.

I already submitted a pull request to go down the route of fix option #1, but I figure I would create a github issue to track this bug and discuss which route to go.

What version of Home Assistant Core has the issue?

core-2024.1.3

What was the last working version of Home Assistant Core?

No response

What type of installation are you running?

Home Assistant Container

Integration causing the issue

opower

Link to integration documentation on our website

https://www.home-assistant.io/integrations/opower/

Diagnostics information

No response

Example YAML snippet

No response

Anything in the logs that might be useful for us?

2024-01-13 02:53:36.696 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating sensor data with: []
2024-01-13 02:53:36.696 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_elec_4000542376_energy_cost and opower:pse_elec_4000542376_energy_consumption
2024-01-13 02:53:38.269 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_elec_4000542376_energy_cost and opower:pse_elec_4000542376_energy_consumption
2024-01-13 02:53:38.967 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Updating Statistics for opower:pse_gas_4000542390_energy_cost and opower:pse_gas_4000542390_energy_consumption
2024-01-13 02:53:40.209 DEBUG (MainThread) [homeassistant.components.opower.coordinator] Finished fetching Opower data in 6.903 seconds (success: True)

Additional information

No response

home-assistant[bot] commented 10 months ago

Hey there @tronikos, mind taking a look at this issue as it has been labeled with an integration (opower) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of `opower` can trigger bot actions by commenting: - `@home-assistant close` Closes the issue. - `@home-assistant rename Awesome new title` Renames the issue. - `@home-assistant reopen` Reopen the issue. - `@home-assistant unassign opower` Removes the current integration label and assignees on the issue, add the integration domain after the command. - `@home-assistant add-label needs-more-information` Add a label (needs-more-information, problem in dependency, problem in custom component) to the issue. - `@home-assistant remove-label needs-more-information` Remove a label (needs-more-information, problem in dependency, problem in custom component) on the issue.

_{^{(message by CodeOwnersMention)}}

opower documentation opower source _{^{(message by IssueLinks)}}

c0mputerguru commented 10 months ago

I've coded up the changes to the HA integration for both options 1 and 2:

tronikos commented 10 months ago

Interesting. I also have solar but I only have 1 meter. I'd prefer we go with id or uuid. Regardless, given the number of users, I'm afraid we need to keep backwards compatibility at least for the statistics which are used in the energy dashboard. One solution is in _insert_statistics go over async_get_accounts twice and if there are duplicates with the same utility_account_id use the id or uuid. But what about the _energy_consumption suffix? Alternatively, continue using utility_account_id but sum the cost reads?

c0mputerguru commented 10 months ago

I get you about backwards compatibility. The solution for that is a lot uglier, but understand not wanting to break everyone already using the dashboards. I don't think we can fully achieve backwards compatibility; if there are currently users that are in the same boat as me (2 data streams with the same utility_account_id), they are currently getting a non-deterministic data stream as it depends on the order in which opower returns the data. I think breaking them so they get a deterministic data stream is actually a better option in this case.

Summing across utility account id also doesn't make sense as my first stream is grid consumption, and 2nd stream is solar production. For the solar production, some of that is consumed, and some of it is fed back to the grid.

Let me take a stab at a mostly backward compatible uuid change and I'll update this thread with a PR.

c0mputerguru commented 10 months ago

I've updated the uuid branch with what I think will work. I still nee to do testing, run precommits, but if you're curious, take a look.

https://github.com/home-assistant/core/compare/dev...c0mputerguru:home-assistant-core:uuid-in-ha-entity-id

tronikos commented 10 months ago

How are you planning on handling the _energy_consumption suffix? Shouldn't one be consumption while the other production? Is there any marker which account is which?

For me on PG&E with one meter, opower provides what value the meter sees, i.e. house consumption minus solar production. So if solar overproduction is fed back to the grid it provides negative values, if my house consumption is all met by solar production it provides 0 value. Summing the 2 values (assuming the solar production is represented as negative) makes sense to me and it will be consistent with the current behavior on the most common setup with just 1 meter.

c0mputerguru commented 10 months ago

Ah, I now understand what you mean by _energy_consumption suffix; that it doesn't make sense for my solar production data stream. Unfortunately I don't see any marker/metadata returned by the API that allows me to distinguish consumption vs production, so I don't see a good way to distinguish. From what I can tell, the naming is purely to make it human readable, so while it is confusing in my dashboard to show _energy_consumption for my production, I'm ok with that level of confusion.

It sounds like you have a net meter data stream. PSE has 2 data streams; 1 which is grid consumption (positive values) and 1 that is raw solar production (positive values). Grid consumption is whatever my home is consuming in addition to what my solar production is. The raw solar production values are what my panels are producing, some of which might be fed back to the grid, and some of which might be consumed. Doing summing (or subtracting since both are positive) doesn't make much sense for those 2 streams. Seems that there just isn't any uniformity in implementation across utilities, which is quite a shame.

I had a chance to test out my changes, see the screenshot of an energy dashboard. I'll be submitting a pull request shortly.

pse multi-meter account

c0mputerguru commented 10 months ago

Thanks @tronikos. Can you also look at the related doc update? https://github.com/home-assistant/home-assistant.io/pull/30974

c0mputerguru commented 10 months ago

Also, I was thinking a bit more, and its ultimately up to you as the owner, but if you wanted to eventually move all of the IDs over to using the UUID, we could publish under both the old and new for some time and use a "in a future release this will be deprecated" warning to move their automations/dashboards over to the UUID.

tronikos commented 10 months ago

I prefer we stick on the current id that matches what users see on their bill than switching to some arbitrary uuid.

c0mputerguru commented 10 months ago

Sounds good to me.

On Tue, Jan 23, 2024, 11:13 AM tronikos @.***> wrote:

I prefer we stick on the current id that matches what users see on their bill than switching to some arbitrary uuid.

— Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/108260#issuecomment-1906754569, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFPT7N56FP4OCA5UF6PDSOTYQADXJAVCNFSM6AAAAABB7GLGSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBWG42TINJWHE . You are receiving this because you authored the thread.Message ID: @.***>

issue-triage-workflows[bot] commented 7 months ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

c0mputerguru commented 7 months ago

This is not stale but waiting on a code review to be completed.

On Mon, Apr 22, 2024, 1:05 PM issue-triage-workflows[bot] < @.***> wrote:

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

— Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/108260#issuecomment-2070859574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFPT7NZQIGHXX3VREV2EZGDY6VUQXAVCNFSM6AAAAABB7GLGSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZQHA2TSNJXGQ . You are receiving this because you authored the thread.Message ID: @.***>

c0mputerguru commented 6 months ago

The bump to the new version and using the account id was reverted (https://github.com/home-assistant/core/pull/117476), so this issue is still unresolved.

c0mputerguru commented 6 months ago

So after all the back and forth, I think the guiding principle should be to have the statistics ID and sensor ID be fully unique, deterministic an should not change if meters get added or removed.

I propose the following:

The statistics ID be opower:{utility}_{type}_{uuid}_{reading/cost} It is currently opower:{utility}_{type}_{account_id}_{consumption/cost} which is not unique.
Sensor unique ID be {utility}_{uuid}_{description.key}. It is currently {utility}_{account_id}_{description.key} which is not unique.
Statistics name be {utility} {type} {reading/cost} if there is only a single meter per meter_type across all accounts, {utility} {type} {account_id} {reading/cost} if there are multiple meters per meter_type across different accounts, and {utility} {type} {uuid} {reading/cost} if there are multiple meters per meter_type within a single account.
Sensor names be {utility} {description.name} if there is only a single meter type for a given type across all accounts, {utility} {account_id} {description.name} if there are multiple meters per meter_type across different accounts, and {utility} {uuid} {description.name} if there are multiple meters per meter_type within a single account.

Does this seem reasonable? If so, I can take a stab at coding up that change, but given that its somewhat complex, I don't want to get started until there's full agreement.

@frenck @emontnemery Do you have any examples of how to migrate statistics IDs or sensor IDs? I'm sure it's had to be done before, and having a good example of the migration process would be helpful. I was thinking that we publish under both the old and new for some period of time, add a notification for integration users that the old one is being deprecated in a future version, and 6 months later take out the old one.

tronikos commented 6 months ago

This will be a breaking change, which I really want to avoid. Having separate meters is rare. The most common case for solar is to have a single bi-directional meter. It doesn't make sense to me for utilities to add a new meter instead of replacing the existing one with a bi-directional one which takes less space and should be cheaper if you take into account labor, box and other materials. Most installed meters in US should already be bi-directional even for those without solar. I had solar installed last year and the meter didn't need to be replaced since it was already bi-directional. According to analytics there are 1843 installations. Since 1/3 are estimated to have enabled analytics, there are ~5.5k installations. You seem to be the only one that has multiple meters.

To me the reverted PR was fine. If meters are added to an account (which is very unlikely), yes the unique ID will change but that's fine IMO. You don't want to lose historical data. Note the opower integration creates sensors for forecast data (very few utilities provide these) and statistics for the energy dashboard. Sensor entities for the forecast data will be orphaned but that's fine IMO. They can easily be deleted and this to happen is even more unlikely (rare to have multiple meters, uncommon for utility to provide forecast sensors).

superm1 commented 6 months ago

Can we have an opt in for those of with multiple meters? Everyone in Austin with solar does for example.

c0mputerguru commented 6 months ago

I'm not sure it's true that the most common case for solar is a single bi-di meter. I have a physical bi-di meter on my home, but they also added a 2nd physical meter to track production as when I had mine installed, our state offered a production incentive. I was talking to a friend who has a different utility who is planning to get solar, and his utility also models their data stream through opower as 2 meters on a single account. Looks like Austin's energy utility is similar.

My bi-di meter also has several data streams (grid pull, solar return, net) back to the utility, but only the 1st is modeled in opower. So I suspect that utilities have the ability to provide richer information to opower but may not be doing it today, but as systems get more sophisticated and there are more solar installs, I wouldn't be surprised if utilities get more data-rich. So even if the majority case today is a single meter, I don't think it's fair to assume that it will always be the case.

I understand the desire to avoid a breaking change, but I thin continuing to keep the statistics ID for a single meter {utility}_{type}_{account_id}_{consumption/cost} cannot meet the goal of being unique and deterministic in the case that a meter is added to the account in the future. So from my perspective, I've got an impossible set of requirements to meet.

I expect that 5.5k number to only grow over time as more people start using HA (I just convinced my dad to use HA the other day), more people start caring about their energy consumption, and more people get energy from multiple sources like solar. I think its better to bite the bullet now and make the breaking change with a migration strategy rather than have to do it when that 5.5k number is 50k and then there's a non-trivial number of users that are coming across IDs changing as their energy sources change.

I've got my personal opinion of what we should do (move to UUID), but at the end of the day, I'm not the library maintainer, the integration maintainer, or an owner of Home Assistant, so I don't ultimately make any decisions. For now, personally I'm running a forked copy of the integration to meet my needs, and I'm happy to help implement whatever is necessary in the main integration, but the decision makers need to come to some sort of agreement first.

asiotia commented 6 months ago

I am a newbie on HA and am having similar issues with PSE and multiple meters because of Solar panels.

My electricity bill has three rate schedules:

Residential (Meter # x4963)
Energy to PSE (Meter # x4963)
Production (Meter # x5060)

I believe my total consumption would be #3 + (#1 - #2). However, opower just gives me #3 and the meter number doesn't match any of the above. I would appreciate any guidance on how to resolve this.

emontnemery commented 6 months ago

@c0mputerguru there are many examples of migrating the unique_id of sensors, search the core repo for async_update_entity.

There are however no examples in the core repo yet of migrating statistics which are not backed by an entity_id; to do that you need to call async_update_statistics_metadata.

I'd be more than happy to help you implement the migration if you message me on discord, I'm emontnemery there too.

c0mputerguru commented 6 months ago

Thanks @emontnemery for the offer of help. I'll definitely take you up on it if I end up working on this. The async_update_statistics_metadata function will indeed be useful to do the actual migration. My questions more were along the lines of what the process would be as changing unique id does break anything that relies on the id like dashboards and automations. So is there a standard process like "announce breaking change, add log message that things will break, wait 6 months, do migration" that you recommend? If it's easier, I can also jump on discord and pick your brain there instead.

That said, I think there is currently a disagreement between the HA owners and the library/integration owners that needs to be resolved first in terms of which direction to take. I'd rather wait to make any implementation changes until we sort that out as I don't want to implement more that eventually just gets thrown away.

If y'all can come to agreement on direction, I'm happy to do the implementation.

emontnemery commented 5 months ago

My questions more were along the lines of what the process would be as changing unique id does break anything that relies on the id like dashboards and automations.

The unique_id of an entity is not visible to the user, it's only there to map something identifying an entity, for example a serial number, to the entity_id which the user sees and can change, for example sensor.blabla. Changing the unique_id of an entity does not break anything because the entity_id is not changed.

tronikos commented 5 months ago

The current statistic id doesn't include the uuid. It includes the account id (matches what you see in the billing statement). Renaming the statistic id will be a breaking change. It's used in the energy dashboard (which in theory we could auto-migrate) and I'm personally using it in many custom graphs (which is harder to auto-migrate). Others might also use the statistic ids in systems outside of HA, such as Grafana. IMO, we should avoid such a breaking change. In the uncommon case one goes from 1 to 2 meters it's OK at that time to create new statistic ids with uuid in them and require users to setup their energy dashboard again. They would have to do that anyway to include data for the 2nd meter. The same if they go from 2 to 1 meter. They would have to reconfigure anyway. IMO, we should re-merge #117330

As a reminder, the integration creates:

entities for forecasted usage/bill (few utilities provide this), and
statistics ids not backed up by any entity

I'm OK changing the unique_id of the entities in 1. This isn't a breaking change. If needed, I'm actually ok with any breaking change for 1. I'm not OK with any breaking change for statistic ids in 2, unless absolutely necessary.

emontnemery commented 5 months ago

@tronikos I would prefer the following solution:

Migrate both unique_id of entities and statistic_id for data points not tied to an entity
Inform users via a repair issue that the statistic_id has changed, and they need to update the configuration of the energy dashboard and other frontend cards which can consume statistics

The following solution would also be OK:

Migrate unique_id of entities
For new opower config entries:
- Make the statistic_id stable so it doesn't change if a second meter is added to or removed from the user's account
For already existing opower config entries
- Only change the statistic_id if the account has more than one meter
- Inform users via a repair issue that the statistic_id has changed, and they need to update the configuration of the energy dashboard and other frontend cards which can consume statistics

tronikos commented 5 months ago

@emontnemery as I said I don't want to introduce a breaking change unless absolutely necessary and I don't think this is such a case so I don't like the first solution.

Your second solution sounds good minus the 2nd bullet. I'd personally say not bother with your second bullet (For new opower config entries) to keep the code simple. I expect going from 1 to 2 meters to be very uncommon so I don't think it's worth complicating the code just for this case. As I said earlier users will have to reconfigure their dashboard to use the new meter anyway. They might as well do it for their existing meter too. In fact they might have to do it anyway since I don't think there is any guarantee the old meter will continue being used for consumption.

@c0mputerguru can you do 1st and 3d bullet of emontnemery's 2nd solution?

c0mputerguru commented 5 months ago

As I said, I'm happy working on any implementation as long as there's agreement amongst all parties. I don't want to spend more time working on an implementation that ultimately gets thrown away.

That said, I think the code will be fairly simple as @emontnemery stated where only existing entities maintain backwards compatibility. Basically we'd do a "if integration already exists and only 1 meter do old logic, else do new logic" sort of implementation.

On Fri, Jun 14, 2024, 11:16 PM tronikos @.***> wrote:

@emontnemery https://github.com/emontnemery as I said I don't want to introduce a breaking change unless absolutely necessary and I don't think this is such a case so I don't like the first solution.

Your second solution sounds good minus the 2nd bullet. I'd personally say not bother with your second bullet (For new opower config entries) to keep the code simple. I expect going from 1 to 2 meters to be very uncommon so I don't think it's worth complicating the code just for this case. As I said earlier users will have to reconfigure their dashboard to use the new meter anyway. They might as well do it for their existing meter too. In fact they might have to do it anyway since I don't think there is any guarantee the old meter will continue being used for consumption.

@c0mputerguru https://github.com/c0mputerguru can you do 1st and 3d bullet of emontnemery's 2nd solution?

— Reply to this email directly, view it on GitHub https://github.com/home-assistant/core/issues/108260#issuecomment-2169155025, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFPT7N7VGFV46BFWGLRS2DTZHPL25AVCNFSM6AAAAABB7GLGSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRZGE2TKMBSGU . You are receiving this because you were mentioned.Message ID: @.***>

tronikos commented 5 months ago

The condition should actually be: if statistics already exist and only 1 meter, continue using the same statistics. It adds a bit of complexity to the code and to the documentation only to help an uncommon case. In addition, as I said, I don't think there is any guarantee the old meter will continue being used for consumption. We will always need to create a repair issue whenever the number of meters changes. So in the end it doesn't help much the uncommon case. I'd say start implementing the 1st and 3d bullets and if @emontnemery strongly wants the 2nd bullet we can discuss further.

vazquezrodjoel commented 5 months ago

Newbie here, but I think I have this situation in my case. I use Evergy (in Kansas City) and they are requiring new solar installations to have 2 meters, one for net metering and the other just for the production of the solar panels. This is a requirement as of this year (not sure if it was January or March, but around there). Anything I can do to try fix this (can't develop but can-do testing if needed).

kreene1987 commented 5 months ago

Adding my voice, I have an old account for a prior house within the same company, and a new account for my current residence. I am getting the data from the old account as it's probably a lower ID. Obviously need new account for current info.

Happy to troubleshoot as time/necessity allows. I am also on Evergy in KC but no solar.

briodan commented 4 months ago

running into a similar issue as kreene1987 but on Enmax. I have two accounts one for an old house one for a new one.

issue-triage-workflows[bot] commented 1 month ago

There hasn't been any activity on this issue recently. Due to the high number of incoming GitHub notifications, we have to clean some of the old issues, as many of them have already been resolved with the latest updates. Please make sure to update to the latest Home Assistant version and check if that solves the issue. Let us know if that works for you by adding a comment 👍 This issue has now been marked as stale and will be closed if no further activity occurs. Thank you for your contributions.

home-assistant / core