-
Notifications
You must be signed in to change notification settings - Fork 13.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logger: add logger_status orb message #14050
Conversation
2af6a68
to
4bb6b5c
Compare
I had some other questions and ideas here about things like broadcasting status (eg is logger ready to start before arming) and capturing dropout duration differently for the log rather than the way it's reset with logger status now, but I'd rather get some basic in now we can continue to improve incrementally. |
4bb6b5c
to
e302e54
Compare
Codecov Report
@@ Coverage Diff @@
## master #14050 +/- ##
==========================================
- Coverage 53.63% 52.89% -0.74%
==========================================
Files 605 605
Lines 51327 51346 +19
==========================================
- Hits 27528 27162 -366
- Misses 23799 24184 +385
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a status is ok, but it won't help with #13200, and most information you're adding is redundant and can already be inferred from the log.
I know, I addressed that above. Next I'll extend this to publish status pre-logging to handle things like #13200 (it's actually a fairly common request). I also think it's going to be worth it to casually monitor logging rate and buffer usage over time across a large range of configurations and different phases of flight. Yes we could get it from the log itself, but we largely don't. Somewhat related, I've seen a number of logs that look like data was missed, but don't have dropouts. I'm wondering about tracking generation gaps in full rate topics (no interval configured) and exposing it here. The overall priorities need a review. |
// update buffer statistics | ||
for (int i = 0; i < (int)LogType::Count; ++i) { | ||
if (!_statistics[i].dropout_start && (_writer.get_buffer_fill_count_file((LogType)i) > _statistics[i].high_water)) { | ||
_statistics[i].high_water = _writer.get_buffer_fill_count_file((LogType)i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove this? This is now broken.
Please don't just merge PR's w/o review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, yes it's clearly bogus, I have it fixed in the wip followup, but I can bring it to master sooner if needed. I apparently screwed up the rebase.
I would much prefer everything goes into master with a real review, but at the moment quite a lot goes ignored. I'd be up for establishing a more thorough system, but we'd need to do something like figure out code owners (and backups), commit to reasonable response times, etc. I also don't want to encourage more "rubber stamp" code reviews where we have the illusion of good process with the overhead and bureaucracy, but hardly any of the actual benefit.
- this was accidentally dropped during a rebase of #14050
Resurrecting this old branch I never finished. The idea is to expose logger status over orb both for the simplicity of monitoring over a regular flight and for deeper integration with the state machine (optional arming requirements, #13200).
WORK IN PROGRESS