Fix hard coded constraint on pipeline name for metrics #11777
Conversation
|
Jenkins test this please |
There was a problem hiding this comment.
Looks to work as advertised. I've left a note about using Pipeline#system?, along with a couple coding-style nitpicks.
I've also raised a question about intended behaviour of the monitoring.cluster_uuid setting, since it is apparently ignored in most cases in the current implementation (we may address or punt this, but I believe it is worth discussing).
| cluster_uuids = result[:cluster_uuids] | ||
| cluster_uuids = [] | ||
| agent.running_pipelines.each do |pipeline_id, _| | ||
| unless pipeline_id.to_sym == :".monitoring-logstash" |
There was a problem hiding this comment.
the yielded Pipeline responds to Pipeline#system?, which is true for the monitoring pipeline -- would using it be safer than hard-coding a skip pipeline?
Secondly for readability I would prefer to special-case the skip clause as next if ... instead of providing a block to unless, since unless blocks are inherently harder to track, e.g.,
agent.running_pipelines.each do |pipeline_id, pipeline|
next if pipeline.system?
# ...
end| end | ||
| end | ||
|
|
||
| unless cluster_uuids.empty? |
There was a problem hiding this comment.
Nitpick: I understand the desire to minimize the diff, but unless...else blocks can get pretty confusing 😩
| cluster_uuids = [] | ||
| agent.running_pipelines.each do |pipeline_id, _| | ||
| unless pipeline_id.to_sym == :".monitoring-logstash" | ||
| path = [:stats, :pipelines, pipeline_id.to_sym, :config] | ||
| found_cluster_uuids = stats.extract_metrics(path, :cluster_uuids) | ||
| if found_cluster_uuids && !found_cluster_uuids[:cluster_uuids].empty? | ||
| cluster_uuids |= found_cluster_uuids[:cluster_uuids] | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Since we have the pipeline object at hand, we might as well just fetch the cluster uuids directly from it.
This way we can get rid of the :cluster_uuid gauge metric that has been causing the warning we see at startup.
| cluster_uuids = [] | |
| agent.running_pipelines.each do |pipeline_id, _| | |
| unless pipeline_id.to_sym == :".monitoring-logstash" | |
| path = [:stats, :pipelines, pipeline_id.to_sym, :config] | |
| found_cluster_uuids = stats.extract_metrics(path, :cluster_uuids) | |
| if found_cluster_uuids && !found_cluster_uuids[:cluster_uuids].empty? | |
| cluster_uuids |= found_cluster_uuids[:cluster_uuids] | |
| end | |
| end | |
| end | |
| cluster_uuids = agent.running_pipelines.flat_map do |_, pipeline| | |
| next if pipeline.system? | |
| pipeline.resolve_cluster_uuids | |
| end.compact.uniq |
| end | ||
| end | ||
|
|
||
| unless cluster_uuids.empty? |
There was a problem hiding this comment.
| unless cluster_uuids.empty? | |
| if cluster_uuids.any? |
|
@robbavey since master and 7.x are currently broken due to a PR merged today that removes the gauge, can you rebase this PR against master to check we're back to green after these modifications? |
Fix for #11775