Trigger latency monitoring configuration #126
Conversation
There was a problem hiding this comment.
I think an entire new class to wrap a boolean is a bit of an overkill, even if it should allow to switch monitoring for all classes at the same time. Once we want to e.g. have monitoring off for TPDataProcessor and on for MLT, we will effectively need to create a new object and edit individual application's data anyway. So might as well let them have individual boolean attributes set to some defaults.
E.g. inserting a boolean into DataProcessor probably would have been enough, or maybe even into SmartDaqApplication (through should probably be careful what we put there)? I'm curious what @glehmannmiotto and @gcrone think here, they have more experience with OKS and can probably think of a more elegant solution.
There are also some syntax problems that I highlighted.
| <class name="LatencyMonitoringConf"> | ||
| <attribute name="enable_latency_monitoring" type="bool" init-value="true"/> | ||
| </class> |
There was a problem hiding this comment.
Personally I think an entire new class to overlay a boolean is a bit of an overkill.
On one hand being able to switch all the monitoring on with one object is nice, but on the other hand, it adds extra complexity when we want to switch only one of these on. And I imagine e.g. by default we might it off, say, RawDataProcessor and TPDataProcessor (especially since now it's on per-TP, not per-TPSet basis) but on in the TCDataProcessor and in the MLT. It could also be added in DataProcessor, so it doesn't need to be in all the derived processors separately.
Perhaps @glehmannmiotto has a more elegant solution here? Perhaps enabling latency monitoring via application? There are other non-DataProcessor-derived objects that also need this.
There was a problem hiding this comment.
I agree with your assessment @ArturSztuc, and think that we should only add a boolean attribute. The question for @MRiganSUSX is who is in charge of the monitoring. Is it the application, the module or the data processor? Depending on this we should put the attribute in the right class (DataProcessor may be the right one). If you are worried about the ease of enabling/disabling this monitoring feature (although, why would you ever want to disable it?), then it is easy to add a python utility in daqconf that changes the value of the attribute for an application, a class of applications, or all.
| @@ -169,6 +170,7 @@ | |||
| <attribute name="td_out_of_timeout" type="bool" init-value="true" is-not-null="yes"/> | |||
| <attribute name="buffer_timeout" type="u32" init-value="100" is-not-null="yes"/> | |||
| <attribute name="td_readout_limit" type="u32" init-value="1000" is-not-null="yes"/> | |||
| <relationship name="latency_monitoring_conf" class-type="LatencyMonitoringConf" low-cc="zero" high-cc="zero" is-composite="yes" is-exclusive="yes" is-dependent="yes"/> | |||
There was a problem hiding this comment.
Shouldn't this be
| <relationship name="latency_monitoring_conf" class-type="LatencyMonitoringConf" low-cc="zero" high-cc="zero" is-composite="yes" is-exclusive="yes" is-dependent="yes"/> | |
| <relationship name="latency_monitoring_conf" class-type="LatencyMonitoringConf" low-cc="one" high-cc="one" is-composite="no" is-exclusive="no" is-dependent="no"/> |
one, otherwise we're saying "I want exactly no LatencyMonitoringConf objects", so what's the point of having them?
is-composite="no" because this is definitely not a composite class. LatencyMonitoringConfig is made of one boolean, not entirely of other objects.
is-exclusive="no" presumably because we want to share this object? Otherwise there's no point of making it a separate class in the first place.
is-dependent="yes" is one that I'm unsure about, I always assumed it's only for objects that go into composite classes. Perhaps @gcrone knows better here.
|
After discussions with @ArturSztuc, this approach is now obsolete and new PR is in #141. |
More details in trigger PR: DUNE-DAQ/trigger#342.