cloudalchemy / ansible-node-exporter

Provision basic metrics exporter for prometheus monitoring tool
MIT License
501 stars 270 forks source link

Pass arbitrary command line flags to node_exporter #154

Closed paol closed 1 year ago

paol commented 4 years ago

What is missing? A way to pass arbitrary command line flags to node_exporter

Why do we need it? node_exporter has more parameters than those exposed by the variables in this role. An "escape hatch" to pass any flags would be very useful. Something like:

node_exporter_flags: ["--web.telemetry-path=/foo", "--web.max-requests=99"]

In fact, with this variable in place, node_exporter_web_listen_address could be deprecated IMO as it would be adequately covered by the more general node_exporter_flags.

paulfantom commented 4 years ago

Whole idea behind having one variable per flag is to have a mechanism which quickly prevents misconfiguration of node_exporter before ansible even gets to install stage. This is covered in tasks/prefilight.yml. If there are mappings which are missing I will happily accept PRs with new additions.

That said I agree about deprecating node_exporter_web_listen_address variable, but the replacement should probably follow pattern done also for collectors. Which in this case would look something like:

node_exporter_web:
  listen-address: <<sth>>
  max-requests: <<sth>>
  telemetry-path: <<sth>>

However this would need to be done in backward-compatible way, to still allow node_exporter_web_listen_address for few releases.

/cc @SuperQ wdyt?

paol commented 4 years ago

That would work too, though it's higher maintenance if more flags are added in the future.

I'll note that node_exporter refuses to start with incorrect/unknown flags, so there is no risk of a mistake going unnoticed.

paulfantom commented 4 years ago

I'll note that node_exporter refuses to start with incorrect/unknown flags, so there is no risk of a mistake going unnoticed.

True, but the current mechanism is more about upgrades than the installation itself. A simple scenario which role wouldn't be able to prevent with free-form flags option is this:

  1. node_exporter is already installed
  2. user sets incorrect flags and runs ansible role
  3. systemd service is updated and restarted
  4. node_exporter is in crash-loop

Above causes downtime. In contrast, failing fast in preflight.yml doesn't update anything on host and prevents incorrect node configuration.

SuperQ commented 4 years ago

The problem with node_exporter_web is Ansible. Since it doesn't have the concept of attribute merge, you can't have a change to telemetry-path without also having to include the other settings as well. Sometimes you want to have one flag setting at one level of group var, and another setting in another group var or host var.

Adding support for the new node_exporter flags is probably what we want.

paol commented 4 years ago

Ah, that's problem that I hadn't considered, and also applies to my original idea.

In that case having separate variables for each flag is the only way to go. Consider this enhancement request officially changed :)

paulfantom commented 4 years ago

In that case having separate variables for each flag is the only way to go. Consider this enhancement request officially changed :)

PRs for new variables are welcome :)

ritwiksin commented 4 years ago

how do I pass the flags? for example collector.supervisord.url

paulfantom commented 4 years ago

@ritwiksin :

Since this is a collector, you can use similar syntax to textfile collector example in https://github.com/cloudalchemy/ansible-node-exporter/blob/0460c128f8d861d6aa28df83fb15226dfec54269/defaults/main.yml#L14-L17

In your case it will be:

node_exporter_enabled_collectors:
  - supervisord:
      url: <SOME_URL>
paulfantom commented 4 years ago

Adding support for the new node_exporter flags is probably what we want.

In that case having separate variables for each flag is the only way to go. Consider this enhancement request officially changed :)

@paol which flags would like to be included? Can you create a PR?

paol commented 4 years ago

All of them? I mean, what I wanted to do when I originally filed this bug was to pass --web.disable-exporter-metrics, but in principle all flags should be settable.

The ones currently missing seem to be: --web.telemetry-path --web.disable-exporter-metrics --web.max-requests --log.level --log.format

Re: PR, I don't have the time to pick this up right now, sorry.

paulfantom commented 3 years ago

--web.telemetry-path is added in https://github.com/cloudalchemy/ansible-node-exporter/pull/196

SuperQ commented 1 year ago

This role has been deprecated in favor of a the prometheus-community/ansible collection.