prometheus-nats-exporter icon indicating copy to clipboard operation
prometheus-nats-exporter copied to clipboard

proposal: refactory to use less/no reflection

Open caarlos0 opened this issue 5 years ago • 2 comments

Hi everyone, thanks for the great work on NATS and on this exporter!

So, I was looking in issues like #37 and #39, and I was looking forward into implementing it, but I found some issues that, while not blockers, would make the code smell bad.

The exporter currently uses reflection to determine the metrics, so, we have a generic exporter, to which we pass an endpoint and it exposes all metrics as gauges.

Some problems in there:

  • we may have metrics better exposed as other types (eg: counters)
  • the metrics don't have the HELP statement

This is kind of OK for the current scope, but, if we are going to expose streaming metrics as well, this will be a bigger issue.

On the reflection side, for example, if we want channel subscription metrics (streaming/channelsz?subs=1), the actually metrics are under subscriptions on the json, not in the root, so we will need to keep "going down" until we reach the actual metrics.

My proposal is:

  • get rid of the reflection in favor of a simpler solution, parsing the JSON output into structs and etc
  • add the missing HELPs
  • finally, start adding the streaming metrics

We can still have plenty of "common code" (http request and etc), we just will have more declarative prometheus.Desc and the parsing will be made to the appropriate structs instead of map[string]interface{}.

What do you think? If you agree, I can work on a prototype...

caarlos0 avatar Oct 17 '18 19:10 caarlos0

I think this is a good idea. It'll require more maintenance as new metrics from servers are exposed, but will be the correct way to do this.

ColinSullivan1 avatar Jan 04 '19 18:01 ColinSullivan1

already did that on streaming and connz metrics.

#54 #75

caarlos0 avatar May 17 '19 20:05 caarlos0