vine: add prometheus instrumentation to python bindings.#4433
Conversation
dthain
left a comment
dthain
left a comment
There was a problem hiding this comment.
Sorry, I didn't see this one earlier. Integrating prometheus will add a lot of value. See a few quibbles below:
| This feature requires the optional `prometheus_client` package: | ||
|
|
||
| ```sh | ||
| pip install prometheus_client |
There was a problem hiding this comment.
A slight hesitation about extra manual installs. Another potential hiccup for the user, and also a dependency that we (probably) won't test since it doesn't come along for the ride in testing.
Is Prometheus available via Conda?
How much does the package weigh?
Does it make sense to make it a default dependency?
| `ImportError` when metrics are enabled. | ||
|
|
||
| Each scrape returns gauge metrics derived from the manager status API. Manager | ||
| statistics are exported as `vine_<field>` (for example, `vine_tasks_waiting` |
There was a problem hiding this comment.
Does it make sense to reuse the status report that the manager is generating for catalog updates?
Or is that one too heavyweight?
There was a problem hiding this comment.
If you mean the report as is, we cannot. Prometheus expects this type of gauges to be single values in a time series. Both the catalog status report and Prometheus here are using the same status function (vine_status), only that they present differently the information.
| # If not given, then TSL is not activated. If True, a self-signed temporary key and cert are generated. | ||
| # @param init_fn Function applied to the newly created manager at initialization. | ||
| # @param status_display_interval Number of seconds between updates to the jupyter status display. None, or less than 1 disables it. | ||
| # @param prometheus If True, serve Prometheus metrics on port 9090. |
There was a problem hiding this comment.
Mixing boolean and integer in the same parameter seems likely to cause headaches down the road.
Let's make it easy on the AI and call this prometheus_port and just make it an integer, such that zero means "any port" and 9090 can be chosen explicitly.
There is no avoiding the fact that multiple running taskvine instances will require some port planning/management by the end user...
The prometheus server runs as a separate process from the manager. It gets information using status calls.
cf5db1a
into
cooperative-computing-lab:master
2 participants
For #3827, coffea-casa.
Merge Checklist
The following items must be completed before PRs can be merged.
Check these off to verify you have completed all steps.
make testRun local tests prior to pushing.make formatFormat source code to comply with lint policies. Note that some lint errors can only be resolved manually (e.g., Python)make lintRun lint on source code prior to pushing.