Metrics: add soc and temp#30926
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
QueryEnergythesoc/tempfields are selected directly frommwhile also aggregating over multiple rows, so for non-grouped queries the snapshot value per bucket is effectively undefined; if you truly want the first slot’s value, consider explicitly aggregating (e.g.MINover aCASEontsor similar) rather than relying on a non-deterministic group pick. - The tests in
db_history_test.gousenew(90.0)/new(45.0)which is invalid in Go (newexpects a type, not a value); replace these with a small helper likefunc f(v float64) *float64 { return &v }to construct the pointers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `QueryEnergy` the `soc`/`temp` fields are selected directly from `m` while also aggregating over multiple rows, so for non-grouped queries the snapshot value per bucket is effectively undefined; if you truly want the first slot’s value, consider explicitly aggregating (e.g. `MIN` over a `CASE` on `ts` or similar) rather than relying on a non-deterministic group pick.
- The tests in `db_history_test.go` use `new(90.0)`/`new(45.0)` which is invalid in Go (`new` expects a type, not a value); replace these with a small helper like `func f(v float64) *float64 { return &v }` to construct the pointers.
## Individual Comments
### Comment 1
<location path="core/metrics/accumulator.go" line_range="22-23" />
<code_context>
+ Temp *float64 `json:"temp,omitempty"`
+}
+
+// setSoc keeps the first reading per slot; isTemp selects temperature over soc.
+func (m *Accumulator) setSoc(value float64, isTemp bool) {
+ if isTemp {
+ if m.Temp == nil {
</code_context>
<issue_to_address>
**suggestion:** The `isTemp` boolean flag conflates two concepts and may hurt readability and extensibility.
Using `setSoc` with an `isTemp` flag makes call sites less clear and couples SoC and temperature concerns, which may not scale if you add more per-slot metadata. Consider either separate methods (e.g. `SetSoc(value float64)` and `SetTemp(value float64)`) or replacing the bool with a more descriptive enum-like type so the intent is explicit at call sites.
Suggested implementation:
```golang
Soc *float64 `json:"soc,omitempty"`
Temp *float64 `json:"temp,omitempty"`
}
// setSoc keeps the first SoC reading per slot.
func (m *Accumulator) setSoc(value float64) {
if m.Soc == nil {
m.Soc = &value
}
}
// setTemp keeps the first temperature reading per slot.
func (m *Accumulator) setTemp(value float64) {
if m.Temp == nil {
m.Temp = &value
}
}
func WithClock(clock clock.Clock) func(*Accumulator) {
```
All existing call sites of `setSoc(value, isTemp)` must be updated:
- Calls that previously passed `isTemp == false` should now call `setSoc(value)`.
- Calls that previously passed `isTemp == true` should now call `setTemp(value)`.
If these helpers are meant to be used outside the package, consider exporting them as `SetSoc` and `SetTemp` and updating call sites accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/cc @daniel309 would this also work to store outside temp? |
hmm, I dont think so. Outside temp is independent of a loadpoint. outside temp would be a separate, additional temp field in addition to the loadpoints warm water (or heating water) temp this fied contains. Also, outside temp is independent of a loadpoint itself. Even if there would be two heaters, I would not expect outside temp to be stored alongside the heater. that said, having outside temp in the history would be very nice though. It would allow users to visually correlate heating power consumption and outside temp, which is the core concept of my heating adjustment PR. |
|
We could log the weather tariff "device", doesn't have to be a loadpoint? I'm not particularly happy with having temp/soc both since our devices only ever contain a single value? |
Isn't that required for your PR? |
Right, I missed that. Outside temp is implemented as Tariff (in the pre-req PR about the Openmeteo Tarif), its stored independently from device or loadpoint there already. So scratch my earlier comment. Outside temp is already stored independently with it, for the consumption of all devices/loadpoints that need it. These indicate that they need the Tariff with the "ScaleLoadByTemperatureForecast" feature flag. |
Agreed, but see tradeoff note in description. We need some kind of differentiation inside metrics to later know what this values means (soc or temp). Even after a loadpoint configuration e.g. was deleted or changed. |
|
@copilot resolve the merge conflicts in this pull request |
Resolved by merging |
|
Changed to a single |
|
So… how do we log outside temp? Same approach? And more importantly: whats the documented contract for logging?, i.e. which soc/temp is this? |
are you referring to the potential weather tariff? For loadpoints and batteries (this PR) there is no ambiguity. We always have only one soc/temp value to log there. |
Yes- we could use the same mechanism?
I.e. Avg.? Beginning of slot? End of slot? |
Start of slot. See first bullet in description and code comment on db field definition. |
I'd say yes. Even if we wanted to record multiple temperatures (outdoor & indoor) we could solve this with different meter groups. |
Nitpicking: isheating? |
That sounds like end of slot, not start of slot? |
Heating is a loadpoint-specific term. I picked |
No, it isn't. Check |
|
one thing about the outdoor temperature tariff PR and persistence that I think is relevant for this discussion. Database persistence of the Tariff temperature time series is only via the generic key-value cache table (evcc/server/db/cache/cache.go) type Cache struct { This is to survive restarts. In order to add this to metrics (this discussion here) additional changes (a temperature meter?) would be required |
We dont need a meter for this. We already use the metrics concept for home consumption (calculated) and solar forecast values. There are also no meters involved. It's just tariff derived data we feed into this. The db tables a little confusing ( The only thing we'd need to add it a point in our loop where we set the temp value and decide about which group/name combination we should use for meta data. |
|
Yes. Lets persist if we encounter a temp tariff. That would be the second minimal PR in addition to adding OpenWeather (and of course a custom temp tariff). |
|
@daniel309 willst du die Messwerterfassung- falls es einen Wetter Tarif gibt- schonmal auf diesen Mechanismus umbauen? Das kann mit in den PR zum Wettertarif rein. |
|
klar, ich schaus mir an. melde mich im temperatur PR wenns erledigt ist |
|
What will happen if you set max temp = 60C for example. So SOC 60C => 100%. but you have another heat source lets say Solarthermie that can heat up your water beyond 60C lets say up to 95C. |
adresses #30185
History metrics now record a per-slot SoC/temperature snapshot next to energy. Battery entities log SoC; loadpoints log SoC, or temperature for heating chargers.
soc_tempcolumn onmeters, captured at slot start (first reading wins, no averaging); auto-migrated, existing rows stay NULL.is_tempflag onentitiesmarks whethersoc_tempholds temperature instead of SoC. One value column instead of two mutually-exclusive ones; the discriminator lives where it belongs, on the entity.is_tempfrom theHeatingfeature at write time; battery is always SoC./history/energyJSON exposessocTempper slot +isTempper series; CSV header picks.soc.pctvs.temp.degC. Bucketed reads report the first slot's value (consistent with slot-start semantics); grouped sums omit the snapshot.Out of scope