Add mongodb-query-exporter

[raffis]

Add mongodb-query-exporter to the list of exporters.
The mongodb-query-exporter creates prometheus metrics out of MongoDB aggregations.

[brian-brazil]

Thanks for sharing, it looks like you should be using ConstMetrics. For example https://github.com/raffis/mongodb-query-exporter/blob/c871a06191355d06180dd88ee2ceba8ab68707fb/collector/main.go#L262 is trying to workaround incorrectly using direct instrumentation in an exporter (and more generally I question whether this sort of exporter can actually produce a Counter - everything should be a Gauge). This will be simpler, and ensure series disappear correctly.

You should also be pulling the metrics synchronously at scrape time, not based on a background timer - see https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling

[raffis]

Thanks for sharing, it looks like you should be using ConstMetrics. For example https://github.com/raffis/mongodb-query-exporter/blob/c871a06191355d06180dd88ee2ceba8ab68707fb/collector/main.go#L262 is trying to workaround incorrectly using direct instrumentation in an exporter (and more generally I question whether this sort of exporter can actually produce a Counter - everything should be a Gauge). This will be simpler, and ensure series disappear correctly.

You should also be pulling the metrics synchronously at scrape time, not based on a background timer - see https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling

Appreciate the review 👍 , will think about the counters again but yes I think I have to agree on this that there will probably never be an actual use case for this.

[raffis]

Thanks for sharing, it looks like you should be using ConstMetrics. For example https://github.com/raffis/mongodb-query-exporter/blob/c871a06191355d06180dd88ee2ceba8ab68707fb/collector/main.go#L262 is trying to workaround incorrectly using direct instrumentation in an exporter (and more generally I question whether this sort of exporter can actually produce a Counter - everything should be a Gauge). This will be simpler, and ensure series disappear correctly.

You should also be pulling the metrics synchronously at scrape time, not based on a background timer - see https://prometheus.io/docs/instrumenting/writing_exporters/#scheduling

Your suggestions are now implemented:

• Dropped support for counter
• Using ConstMetrics now
• Pulling synchronously at scrape time
• (Added support for cache instead)

[brian-brazil]

Your cache is broken, as you're concurrently reading and modifying c.cache from different goroutines. Concurrent map modification and access must be protected by a mutex.

I also think you don't need a custom registry here, the default one should suffice.

[raffis]

Your cache is broken, as you're concurrently reading and modifying c.cache from different goroutines. Concurrent map modification and access must be protected by a mutex.

Indeed, good catch.

I also think you don't need a custom registry here, the default one should suffice.

I used a custom one because of testing, with the one at package level you would have to unregister depening on the case. Or did I miss here something?

[brian-brazil]

I used a custom one because of testing, with the one at package level you would have to unregister depening on the case. Or did I miss here something?

I'm looking at your main function, which seems to have an unneeded registry. I didn't look at the tests, but custom registries aren't unusual there.

[raffis]

Your cache is broken, as you're concurrently reading and modifying c.cache from different goroutines. Concurrent map modification and access must be protected by a mutex.

I also think you don't need a custom registry here, the default one should suffice.

It's fixed now in master.

[brian-brazil]

Thanks!

I'd suggest using a defer for the unlock in getCached, as that's going to be more robust in the face of future code changes.

[raffis]

Thanks for the suggestions, appreciate it!