oliver006 / redis_exporter

Prometheus Exporter for ValKey & Redis Metrics. Supports ValKey and Redis 2.x, 3.x, 4.x, 5.x, 6.x, and 7.x
https://github.com/oliver006/redis_exporter
MIT License
3.04k stars 860 forks source link

feat: add option to disable exporting values of keys #809

Closed vin01 closed 1 year ago

vin01 commented 1 year ago

backwards compatible version of https://github.com/oliver006/redis_exporter/pull/806

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 152


Totals Coverage Status
Change from base Build 150: 0.0%
Covered Lines: 1920
Relevant Lines: 2075

💛 - Coveralls
oliver006 commented 1 year ago

Thanks for the PR, this is definitely more in line with what I think makes sense.

Here's a thought or question: your concern with leaking information was with regards to the /scrape endpoint as that would allow anyone with network access to the exporter to extract any string value from the exported/monitored Redis instance. How about a flag that disables the scrape endpoint? By default it'll be on (backwards compatibility...) but it can be disabled and then random key extraction shouldn't be possible any longer. One could argue that if --redis.addr= is NOT set we could disable the /scrape endpoint but this could still affect campatibility with existing setups that rely on /scrape to work even though the exporter is configured to monitor a particular instance. Anyway, let m know what you think but I'm inclined to go with a more broader off-switch for the /scrape endpoint

vin01 commented 1 year ago

Thanks for sharing your thoughts, I actually did consider that but then went for this approach since these two things will then need to be configurable independently of each other.

Similar to blackbox_exporter, in some setups, running a single exporter to scrape multiple dynamically spawned redis hosts/containers and providing target dynamically based on service discovery is how it works. In those cases, enabling /scrape and not allowing key extraction would still be desirable.

However, having an independent option to disable /scrape would also be a good idea for static host setups. Perhaps in another PR. wdyt?

oliver006 commented 1 year ago

However, having an independent option to disable /scrape would also be a good idea for static host setups. Perhaps in another PR. wdyt?

You're very right, wholesale disabling /scrape is not solving the problem, the approach with finer control is required. This is good - lets' do both switches, I'm fine with doing it in one or two PRs, no preference.

oliver006 commented 1 year ago

This PR looks good. Can we change the name of the flag (and related functions), mauybe something like "disableExportingKeyValues" ?