feat(#10415): role labels for prometheus express metrics #10417
Conversation
|
I ran out of time this week to review this PR, but just wanted to say I plan to dig into it next week. 👍 |
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
Code changes here are good and work as expected!
I would prefer to hold off merging this until Diana has weighed in on the issue discussion. 👍
| 45, 90, 180, 360, 600, | ||
| 1200, 1800, 3600 | ||
| ], | ||
| additionalLabels: ['userRoles'], |
There was a problem hiding this comment.
Super minor, but IMHO keeping this underscored instead of camelCase would feel more consistent (since the metric names are underscored...)
| .join(','); | ||
|
|
||
| return { | ||
| userRoles: presentableRoles, |
There was a problem hiding this comment.
I really hate that prometheus-api-metrics does not support emitting multiple time series (with different label values) per request. 😞 My understanding of Prometheus "best practices" would be that we should just have a user_role label and then emit multiple time series when a user has multiple roles (one for each role). This would make it simple to slice and dice the metrics by role. As things are here, we will have to do some regexing in the queries...
Unfortunately, I spent a bunch of time trying alternatives here and none of them were satisfactory. I think what you have here is the best we can do (so no changes needed).
|
This PR is now marked "stale" after 30 days without activity. It will be closed automatically in 10 days unless you add a comment, push new changes or remove the "stale" label. |
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
@kennsippell based on Diana's comment I think we should obfuscate the role names before emitting them on a public endpoint.
Description
Uses the Additional Metric Labels feature to allow segmentation of express prometheus data by user role.
closes #10415
Fxies a related npm build warning
closes #10416
Code review checklist
can_view_old_navigationpermission to see the old design. Test it has appropriate design for RTL languages.Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.