-
-
Notifications
You must be signed in to change notification settings - Fork 420
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
POC: Prometheus metrics module #816
base: master
Are you sure you want to change the base?
Conversation
This adds a module, which will expose a Prometheus scrape target at /server/prometheus/metrics Also registers a simple Info metric with version info and UUID
Instead, we make some assumptions: | ||
- Response will be in the "normal" format and not openmetrics | ||
- No filtering by name will be done | ||
- gzip won't be used (also sparing the CPU cycles in return for bandwidth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn't be difficult to provide the WebRequest
access to the request headers if needed. This hasn't been required to date so I haven't done so.
As far as gzip compression, a reverse proxy could handle that if its desired. I don't think it needs to be in Moonraker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as gzip compression, a reverse proxy could handle that if its desired. I don't think it needs to be in Moonraker.
You are absolutely right. These notes basically address all the parts of prometheus client's default handler that I stripped in order to get this integrated.
) | ||
self.server.register_event_handler( | ||
"server:klippy_disconnected", self._clear_metrics | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moonraker doesn't send a klippy_disconnected
event. It does register a notify_klippy_disconnected
websocket notification that piggybacks off of the server:klippy_disconnect
event.
Thanks, I have a few questions/comments based on your initial feedback.
Is this a limitation in Prometheus? Does it support any type of authentication?
I'm not sure Kevin would be enthusiastic about merging instrumentation directly. Its effectively repackaging the data, sending instrumented objects to Moonraker would duplicate the serialization/deserialization effort, presumably with additional overhead. Part of the motivation for Klipper's API server is to farm this kind of work to applications outside of Klippy.
Status updates can still be received after shutdown, but not after a disconnect. |
No, this is my mistake. I forgot that Moonraker api keys work simply as bearer tokens. That works just fine with Prom.
And this is exactly why I think we'll be pondering this for a while. I understand and share the desire to minimize any extraneous stuff in Klippy itself. But I just don't think this particular thing can be farmed out effectively. In Klippy, there's access to the actual objects in question, type information, introspection, and the like. It should be possible to do most of the instrumentation from the Prometheus module itself (with a little bit of introspection) and not litter every other module manually with instrumentation code. But doing it in Moonraker means inferring all that information which Klippy already has, based on an implicit API (status updates), which doesn't have a clear schema. Any change in Klippy must be followed by a change in Moonraker and a bespoke converter from status JSON to metrics for each component type. That's overhead too. And unlike status updates, which are strictly scheduled, the Prometheus metrics are only serialized during scrape.
I failed to articulate my question clearly. What I mean is: if a status updates comes right before a disconnect, will their handlers still execute in order every time, or is there a risk that after clearing the metrics (on disconnect, on shutdown, etc.) a wayward update handler still runs and writes a value from the old session? |
This is a POC to address #504 . It is meant to spawn feedback, but is still neither complete nor satisfactory to me.
No apikey/auth supportMy bad. Moonraker already handles that in the server.