Add option to serve metrics via http #20
Conversation
|
Hi, Would merge then. |
|
Sorry for the late response, |
We modified the default behaviour to expose Prometheus metrics in HTTP(using the -d flag). Without the -d flag, it behaves as it did before. We also added a few other flags to provide more flexibility.
Check if Gauge exists before creating it. Use the same registry during all the serve blocking process instead of recreating and repopulating it every time.
|
Hello again, |
|
In general, this looks good, thanks! |
There was a problem hiding this comment.
Looking good overall, thanks!
Some minor remarks attached.
Another thought:
We should consider adding a metric with the timestamp of the last update. When using a file, this information is implicitly part of node_textfile_mtime_seconds, but thats not the case when this is served as HTTP.
| "--port", | ||
| type=int, | ||
| default=os.getenv("PKG_EXPORTER_PORT", 8089), | ||
| help="Bind port", |
There was a problem hiding this comment.
The help should display the default port
| "--bind-addr", | ||
| type=str, | ||
| default=os.getenv("PKG_EXPORTER_ADDR", "0.0.0.0"), | ||
| help="Bind address", |
There was a problem hiding this comment.
The help should display the default bind address.
| "-a", | ||
| "--bind-addr", | ||
| type=str, | ||
| default=os.getenv("PKG_EXPORTER_ADDR", "0.0.0.0"), |
There was a problem hiding this comment.
We should bind to a dualstack address, probably ::
| exporter_file = os.getenv("PKG_EXPORTER_FILE", "/var/prometheus/pkg-exporter.prom") | ||
|
|
||
| def write_registry_to_file(registry, exporter_file=None): | ||
| if not exporter_file: |
There was a problem hiding this comment.
I think this is duplicate, exporter_file is already filled with a default in line 87
| default=os.getenv( | ||
| "PKG_EXPORTER_FILE", | ||
| "/var/prometheus/pkg-exporter.prom"), | ||
| help="File to export, if used the content will not be served", |
There was a problem hiding this comment.
In my opinion, the default is "output to a file" anyway? The comment indicates that by default the HTTP Server is run.
| def serve(addr, port, timewait, rootdir): | ||
| start_http_server(addr=addr, port=port) | ||
| while True: | ||
| sleep(timewait) |
There was a problem hiding this comment.
Is there a more elegant way of solving this? I'm not a fan of a static sleep, especially since this does not include the time "populate_registry" takes.
A
-dflag can be used with thepkg-exportercommand to serve metrics via HTTP. We also added a few other flags that can be used to modify the default behavior of the exporter.