QubitProducts / exporter_exporter

A reverse proxy designed for Prometheus exporters
Apache License 2.0
336 stars 55 forks source link

Conflict on 'module' query string? #23

Closed candlerb closed 4 years ago

candlerb commented 4 years ago

Several exporters already make use of a 'module' query argument: e.g.

This means you might have to do:

localhost:9999/proxy?module=snmp&target=10.12.255.1&module=if_mib_secret
                     ^^^^^^^^^^^                    ^^^^^^^^^^^^^^^^^^^^
                  exporter_exporter                    snmp_exporter

This is confusing at first glance, and exporter_exporter has to strip off the first instance of module and leave the remaining instances in place.

I have tested this (with 0.2.9, prior to the parsing changes in #19), and it does work: tcpdump shows the above example proxies to

GET /snmp?module=if_mib_secret&target=10.12.255.1 HTTP/1.1
Host: localhost:9116
User-Agent: Go-http-client/1.1
Accept-Encoding: gzip

However, I wonder if this could be done in a cleaner way? Suggestions:

  1. Make the exporter_exporter module selection be part of the path, REST-style

    localhost:9999/proxy/snmp?target=10.12.255.1&module=if_mib
  2. Use a query string parameter which is unlikely to conflict

    localhost:9999/proxy?exporter_exporter=snmp&target=10.12.255.1&module=if_mib

    (This might be better for dynamic rewrite rules: setting __params_exporter_exporter is easy, but I'm not sure if __metrics_path__ can be rewritten) __metrics_path__ can be set in relabelling, but __param_module cannot be set to a list.

Both could be done in a backwards-compatible way by falling back to the existing 'module' query parameter.

tcolgate commented 4 years ago

I'm not sure this is worth changing. Things work as is, so this is largely aesthetic.

urusha commented 4 years ago

@tcolgate I'm totally agree with @candlerb We're using consul as config storage for prometheus and relabel_config. It allows to override only the first param (module in our case): The __param_<name> label is set to the value of the first passed URL parameter called <name>. So using exporter_exporter + blackbox_exporter + dynamic relabeling is impossible at this moment. That's why we need a fix for this. /proxy/module semantic would be ideal. This looks nice and easy. Please, add this way of querying. Or please tell if the PR implementing this will be accepted?

mrichar1 commented 4 years ago

However, I wonder if this could be done in a cleaner way? Suggestions:

2. Use a query string parameter which is unlikely to conflict
   ```
   localhost:9999/proxy?exporter_exporter=snmp&target=10.12.255.1&module=if_mib
   ```

This second option looks like it would be very easy to implement, by changing the code here to look for a different name/falling back to module[1:] - I'd be happy to try creating a PR for this if it's likely to be accepted?

https://github.com/QubitProducts/exporter_exporter/blob/a954d8cbcbe56a6150b86dc6fe51c33b80641c88/http.go#L52