Aggregator: publish diagnostics_toplevel_state immediately on every degradation #324
Conversation
|
I took the liberty of mimicking this implementation: #197, as that has proven itself in production for a few years now. |
|
Happy new year everyone! As this PR is not backwards compatible with changes as of 2023-12-05, I guess it might be preferred to get this in quickly? |
|
Hi, Sorry for the slow answer, your MR looks good to me. I couldn't really replicate the test issue |
|
You're welcome! |
There was a problem hiding this comment.
Sorry, I have to correct my previous review.
- Please see inline comment
- Please add a test for this behavior in https://github.com/ros/diagnostics/blob/add/cpu_monitor/diagnostic_aggregator/test/test_critical_pub.py#L89-L90
| @@ -182,8 +165,17 @@ void Aggregator::diagCallback(const DiagnosticArray::SharedPtr diag_msg) | |||
| if (!analyzed) { | |||
| other_analyzer_->analyze(item); | |||
| } | |||
|
|
|||
| // In case there is a degraded state, publish immediately | |||
| if (critical_ && item->getLevel() > last_top_level_state_) { | |||
There was a problem hiding this comment.
This could lead to problems, because stale is larger than everything else.
There was a problem hiding this comment.
Yes? Would it be bad to publish immediately if items become stale?
There was a problem hiding this comment.
@ct2034 is this a problem if we publish immediately on stale?
ct2034
added
enhancement
|
I've added the test, ready for a re-review |
|
Subtle ping 🙂 |
|
Since this is technically a breaking change, I would like to have this in rolling before Jade is split off. @ct2034 are you happy with the added test? |
|
Aaaand we missed Jazzy 😿 Can we please get some traction on this now? Or have at least some comments on what needs to be changed? |
|
Monthly ping? |
| @@ -182,8 +165,17 @@ void Aggregator::diagCallback(const DiagnosticArray::SharedPtr diag_msg) | |||
| if (!analyzed) { | |||
| other_analyzer_->analyze(item); | |||
| } | |||
|
|
|||
| // In case there is a degraded state, publish immediately | |||
| if (critical_ && item->getLevel() > last_top_level_state_) { | |||
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
|
Great, thank you! |
…egradation (#324) (#355) * Aggregator: publish diagnostics_toplevel_state immediately on every degradation * Update docs * Re-use the publishData function such that also the actual diagnostics are available * Expand test with more degradation cases (cherry picked from commit dbaec04) Co-authored-by: Tim Clephas <[email protected]>
…egradation (#324) (#356) * Aggregator: publish diagnostics_toplevel_state immediately on every degradation * Update docs * Re-use the publishData function such that also the actual diagnostics are available * Expand test with more degradation cases (cherry picked from commit dbaec04) Co-authored-by: Tim Clephas <[email protected]>
…egradation (#324) (#357) * Aggregator: publish diagnostics_toplevel_state immediately on every degradation * Update docs * Re-use the publishData function such that also the actual diagnostics are available * Expand test with more degradation cases (cherry picked from commit dbaec04) Co-authored-by: Tim Clephas <[email protected]>
Enhancement to: #317
Notes
I chose this option as there has not been a release since then. And yet another backward compatible parameter seemed overdone.
@outrider-jhulas does this work for your situation as well?
@outrider-jhulas I could not get your test to work.
colcon testalways succeeds butlaunch_test diagnostic_aggregator/test/test_critical_pub.pyalways fails.Rationale: we respond quickly to warnings as well as errors. So for consistency all degradation should be published quickly i.m.o.