-
Notifications
You must be signed in to change notification settings - Fork 3
feat: expose prometheus metrics #62
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
Conversation
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
mtojek
left a comment
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.
First review round done.
What dragged my attention was initiator_id property, and the risk of metrics increase if there are many initiators, otherwise only minors.
| Subsystem: "prompts", | ||
| Name: "total", | ||
| Help: "The number of prompts issued by users (initiators).", | ||
| }, append(baseLabels, "initiator_id")), |
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.
How many initiator_ids do we expect? I wanted to confirm - it will not blow up the registry?
EDIT:
actually, why exactly do we need this id? is it to identify tool, user, or something else?
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.
Operators will need to know if one user (or, by extension, agent) is overloading the system.
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.
Ok, but since initiator_id will be uuid, how will operators match and id with specific agent or extension? will bridge support/"deference" it?
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.
initiator_id == users.id
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
Signed-off-by: Danny Kopping <danny@coder.com>
mtojek
left a comment
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.
Nice job, Danny! Nothing blocking on my side, but please wait for 👍 from @pawbana.
| Subsystem: "prompts", | ||
| Name: "total", | ||
| Help: "The number of prompts issued by users (initiators).", | ||
| }, append(baseLabels, "initiator_id")), |
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.
Ok, but since initiator_id will be uuid, how will operators match and id with specific agent or extension? will bridge support/"deference" it?
Upgrades `coder/aibridge` to v0.2.0 which includes coder/aibridge#62. Creates a `prometheus.Registerer` with a prefix `coder_aibridged_` and passes that along to coder/aibridge which actually exposes the metrics. Also includes a side-effect of a change described in coder/aibridge#62 (comment). --------- Signed-off-by: Danny Kopping <danny@coder.com>
Upgrades `coder/aibridge` to v0.2.0 which includes coder/aibridge#62. Creates a `prometheus.Registerer` with a prefix `coder_aibridged_` and passes that along to coder/aibridge which actually exposes the metrics. Also includes a side-effect of a change described in coder/aibridge#62 (comment). --------- Signed-off-by: Danny Kopping <danny@coder.com>
Closes #25
Required for coder/coder#20865
Metrics are updated abstractly, no provider-specific logic: all updates happen in the recorder/interceptor layers.