Monitoring for FragmentAggregatorModule#418
Merged
MRiganSUSX merged 4 commits intopatch/fddaq-v5.3.xfrom Jun 11, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds operational monitoring metrics to the FragmentAggregatorModule, including counters for data requests and fragments, error‐bit tracking, timing statistics, and updated protobuf definitions.
- Removes deprecated opmon methods and variables
- Introduces atomic counters and timing logic in header and implementation
- Defines new protobuf messages for counters and timing metrics
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| schema/dfmodules/opmon/FragmentAggregatorModule.proto | Adds counter and timing message definitions |
| plugins/FragmentAggregatorModule.hpp | Removes old stats, declares new atomic metrics and generate_opmon_data() |
| plugins/FragmentAggregatorModule.cpp | Implements metric updates, error‐bit checks, timing, and publishing logic |
Comments suppressed due to low confidence (3)
plugins/FragmentAggregatorModule.hpp:95
- [nitpick] The field
m_fragments_time_average_usactually accumulates total time, not the average. Renaming it tom_fragments_time_total_uswould more accurately reflect its purpose.
std::atomic<metric_counter_type> m_fragments_time_average_us{ 0 };
plugins/FragmentAggregatorModule.cpp:69
- Consider adding unit tests for
generate_opmon_data()to validate that counters reset correctly and timing metrics publish expected values under various scenarios.
void
FragmentAggregatorModule::generate_opmon_data()
plugins/FragmentAggregatorModule.cpp:160
- Storing the start timestamp in a shared member risks race conditions if methods run concurrently. Consider using a local variable instead of
m_timestamp_before_dr.
m_timestamp_before_dr = get_current_time_us();
mroda88
approved these changes
Jun 11, 2025
Contributor
mroda88
left a comment
There was a problem hiding this comment.
Tested at EHN1 and dashboard are created.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upon request from @mroda88, here is an initial version of opmon monitoring for FragmentAggregatorModule.
This includes:
The counters for
failedare set-up to be cumulative (no resets).Testing:
Data Requests processing times:
Fragments processing times:
Fragments counters:
Data requests counters: