sensu-plugins / sensu-plugins-haproxy

This plugin provides native HAProxy instrumentation for monitoring and metrics collection, including: service health and backend server metrics.
http://sensu-plugins.io
MIT License
11 stars 37 forks source link

req_rate and req_rate_max are only available on FRONTEND metrics #22

Closed hany closed 7 years ago

hany commented 7 years ago

According to the HAProxy manual for 1.5+:

 46. req_rate [.F..]: HTTP requests per second over last elapsed second
 47. req_rate_max [.F..]: max number of HTTP requests per second observed

The [.F..] indicates these stats are only available for FRONTENDs, and as such, never get rendered since metrics-haproxy.rb uses:

      if line[1] == 'BACKEND'
        ...
        unless line[46].nil?
          output "#{config[:scheme]}.#{line[0]}.requests_per_second", line[46]
        end
        unless line[47].nil?
          output "#{config[:scheme]}.#{line[0]}.requests_per_second_max", line[47]
        end
        ...

It appears that metrics-haproxy.rb don't check any FRONTEND metrics. Is this intentional?

majormoses commented 7 years ago

Probably not? I have not checked but I would be open to a PR to include FE metrics.

Evesy commented 7 years ago

It looks like --server-metrics is provided as an additional argument for the purpose of gathering some frontend metrics:

if line[1] == 'BACKEND'
  ...
elsif config[:server_metrics]
  output "#{config[:scheme]}.#{line[0]}.#{line[1]}.session_total", line[7]
  output "#{config[:scheme]}.#{line[0]}.#{line[1]}.session_current", line[4]
end

Adding in the request related metrics to be output if the config option is provided appears to do what you'd want:

$ /opt/sensu/embedded/bin/metrics-haproxy.rb -c <ip> -P 1936 -q stats -u <user> -p <pass> --server-metrics | grep FRONTEND
centos-512mb-lon1-01.haproxy.localnodes.FRONTEND.session_total 0 1500756381
centos-512mb-lon1-01.haproxy.localnodes.FRONTEND.session_current 0 1500756381
centos-512mb-lon1-01.haproxy.localnodes.FRONTEND.requests_per_second 0 1500756381
centos-512mb-lon1-01.haproxy.localnodes.FRONTEND.requests_per_second_max 0 1500756381
centos-512mb-lon1-01.haproxy.localnodes.FRONTEND.requests_total 0 1500756381
centos-512mb-lon1-01.haproxy.stats.FRONTEND.session_total 20 1500756381
centos-512mb-lon1-01.haproxy.stats.FRONTEND.session_current 1 1500756381
centos-512mb-lon1-01.haproxy.stats.FRONTEND.requests_per_second 1 1500756381
centos-512mb-lon1-01.haproxy.stats.FRONTEND.requests_per_second_max 2 1500756381
centos-512mb-lon1-01.haproxy.stats.FRONTEND.requests_total 22 1500756381

The option doesn't really (to me at least) describe its purpose very well though unless I'm misunderstanding it. 'Add frontend metrics (for backend servers)' or something similar makes more sense to me.

  option :server_metrics,
         description: 'Add metrics for backend servers',
         boolean: true,
         long: '--server-metrics',
         default: false

Thoughts on if adding these additional metrics to be output by --server-metrics is sufficient for your problem @hany?

cc @majormoses

majormoses commented 7 years ago

@Evesy yup looks right from what I see looking quickly. I am torn on if we should just enhance the description or actually rename the actual options (making it a breaking change) to better reflect it's intended use case.

Evesy commented 7 years ago

I'd rather not rename option(s) and create a breaking change without input from the plugins original contributors or heavy users; since I don't actually use this plugin.

Even the --backends option doesn't seem correct to me. If your haproxy configuration is divided into specific separate frontend/backend/stats blocks then you're not necessarily querying a backend. Example below both 'fe' & 'stats' I wouldn't really class as backends... I think the correct term according to this documentation is proxies.

image

I vote for a minor release adding in the requested metrics to --server-metrics along with enhancing the description to be a bit more descriptive; and potentially create a separate issue/discussion around what the correct naming conventions should be (And also if metrics-haproxy.rb should be extended a bit more to give equal focus on frontend metrics with the ability to choose between the two)

hany commented 7 years ago

@Evesy that would definitely work for me. I agree with your proposed solution.

majormoses commented 7 years ago

closed via #24