util: make health-check error message static - #6540
Conversation
|
/assign @nixpanic |
|
/assign @Madhu-1 |
|
Thanks for taking this issue! Let us know if you have any questions! |
1 similar comment
|
Thanks for taking this issue! Let us know if you have any questions! |
| c.mutex.Lock() | ||
| c.healthy = false | ||
| c.err = fmt.Errorf("health-check has not responded for %f seconds", delay.Seconds()) | ||
| c.err = errors.New("health-check has not responded for too long") |
There was a problem hiding this comment.
@SanjalKatiyar having the timestamp in the logs helps in debugging, can you please see if we can trip down this error before sending?
There was a problem hiding this comment.
Ideally, I would have liked it as a log inside checker itself, but I see that the AGENTS.md explicitly says not to do that :/
There was a problem hiding this comment.
parsing/trimming doesn't sound too clean, but let me check...
There was a problem hiding this comment.
What about something like, “has not responded since Xxxx” where Xxx is the actual time object. We can deduce duration from the log timestamps while debugging?
There was a problem hiding this comment.
if purpose is debugging (as mentioned by Madhu above), log timestamps are not very human friendly, or easily readable...
There was a problem hiding this comment.
The issue with many reported events is real, but the time when it started is also important to note. I prefer too many events with useful details, over one event that leaves the time of it's occurrence as a kin of scavenger hunt.
Maybe just include the fixed timeout (over and over again), and see if the timestamp of the event is sufficient?
There was a problem hiding this comment.
let me combine both @black-dragon74 and @nixpanic suggestions...
The health-checker reported the duration in its error message, causing the string to change on every poll. The csi-addons sidecar compares consecutive error texts and emits a new Event whenever they defer, resulting in lots of seemingly duplicate volume-condition Events. Signed-off-by: SanjalKatiyar <sanjaldhir@gmail.com>
4abff91 to
c0d08bc
Compare
| c.mutex.Lock() | ||
| c.healthy = false | ||
| c.err = fmt.Errorf("health-check has not responded for %f seconds", delay.Seconds()) | ||
| c.err = fmt.Errorf("health-check has not responded since %s (timeout: %s)", |
There was a problem hiding this comment.
timeout in log is not useful.
There was a problem hiding this comment.
Maybe just include the fixed timeout (over and over again)
@Madhu-1 maybe I misunderstood @nixpanic, but I added it due to this #6540 (comment).
There was a problem hiding this comment.
@nixpanic what is the use of having const timeout in the log, i assume since last time should be enough?
There was a problem hiding this comment.
The timeout makes helps in identifying the time range when the issue started to happen. Users that are troubleshooting the issue will be helped by it.
Is there a good reason to not include it?
There was a problem hiding this comment.
its just a conts keep on repeating and not configurable as well. usually timeouts are not logged. it doesnt hurt as well if we want to include it
|
Wait with |
Merge Protections🔴 1 of 2 protections blocking · waiting on ⛓️ dependency
🔴 ⛓️ Depends-On RequirementsWaiting for
This rule is failing.Requirement based on the presence of
Show 1 satisfied protection🟢 🚦 Auto-queueWhen all merge protections are satisfied, this pull request will be queued automatically. |
nixpanic
added
logging
4 participants
The health-checker reported the duration in its error message, causing the string to change on every poll. The csi-addons sidecar compares consecutive error texts and emits a new Event whenever they defer, resulting in lots of seemingly duplicate volume-condition Events/Logs.
Events:
Addons Sidecar Logs:
CI job ordering.
Depends-on: #6544