Add info why optimizer is not requestingan solution - #31919
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 left some high level feedback:
- The TRACE logs for the optimizer request and result (
%+vonreqandresp) may reveal sensitive data or produce extremely large log entries; consider reducing the detail or guarding these logs behind a more specific flag. - The format string
"Optimizer: battery data not ready for now: %s"is passedsite.batteryMeters, which is not a string; use%v(or a more appropriate formatter) to avoid incorrect output or runtime issues. - Log prefixes are now mixed between
"Optimizer:"and"optimizer:"; consider standardizing the prefix casing to keep logs easy to filter and search.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The TRACE logs for the optimizer request and result (`%+v` on `req` and `resp`) may reveal sensitive data or produce extremely large log entries; consider reducing the detail or guarding these logs behind a more specific flag.
- The format string `"Optimizer: battery data not ready for now: %s"` is passed `site.batteryMeters`, which is not a string; use `%v` (or a more appropriate formatter) to avoid incorrect output or runtime issues.
- Log prefixes are now mixed between `"Optimizer:"` and `"optimizer:"`; consider standardizing the prefix casing to keep logs easy to filter and search.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| gt, err := site.homeProfile(minLen) | ||
| if err != nil { | ||
| site.log.ERROR.Printf("optimizer: failed to get home profile: %v", err) |
There was a problem hiding this comment.
Give a hint about he reason why there is no result from the optimizer.
It might help the user to solve the issue
There was a problem hiding this comment.
The error is already returned. You MUST NOT log and return. Instead, return enriched error if necessary.
| // meters configured but measurements not in yet: retry instead of | ||
| // consuming the slot gate | ||
| if len(site.batteryMeters) > 0 { | ||
| site.log.DEBUG.Printf("optimizer: battery data not ready for now: %v", site.batteryMeters) |
There was a problem hiding this comment.
Similar reason as above, show the reason why you see the optimizer starting to work, but there is no result visible in the UI. this way you know there is some data missing.
| if err != nil { | ||
| return err | ||
| } | ||
| site.log.TRACE.Printf("optimizer: result: %+v", resp) |
There was a problem hiding this comment.
This is already in the HTTP log, remove.
|
Its unclear what this PR does and what value it adds. |
|
The main reason for me is to avoid questions like I had myself: So in total give the user more information if the optimizer does not produce results. |
| site.log.DEBUG.Printf("optimizer: battery data not ready for now: %v", site.batteryMeters) | ||
| return errOptimizerNotReady | ||
| } | ||
| site.log.DEBUG.Printf("optimizer: no battery data skipping") |
There was a problem hiding this comment.
you either have no batteries configured or any of the pervious warnings will have triggered. This one is redundant?
There was a problem hiding this comment.
Agreed, it is somewhat redundant.
This happens only if there is at least one battery, but no battery was added in the for loop above.
So you got the warning for each configured battery concerning the mising capacity/soc.
I still think it is useful to have this additional and final message why the optimizer is not ready, at least to understand you need at least one (you could think "I don't care about the warningt here is no capacity, who cares")
On top I think it makes sense to have before each unsuccsesful return a message why. Let's you always better understand what happend.
For me, understandablilty of the behaviour is more important than 1 line of log output.
Co-authored-by: andig <cpuidle@gmail.com>
2 participants
for now the optimizer quitely stops before isueing a request to OPTIMIZER_URL
Now we get a hint why